Deleting pointers in destructor

Question

I have some pointers that I allocate in the constructor of a class and then attempt to delete in its destructor:

TileMap::TileMap(int x, int y) {

    mapSize.x = x;
    mapSize.y = y;

    p_p_map = new Tile*[x];

    for(int i = 0; i < x; i++) {

        p_p_map[i] = new Tile[y];

    }

    randomize();

}

TileMap::~TileMap() {

    for(int i = 0; i < mapSize.x; i++) {

        delete p_p_map[i];

    }

    delete p_p_map;

}

void TileMap::randomize() {

    for(int i = 0; i < mapSize.x; i++) {

        for(int j = 0; j < mapSize.y; j++) {

            p_p_map[i][j] = *new Tile(Tile::TileSize * i, Tile::TileSize * j, TileType::randomType());

        }

    }

}

At the end of the program the destructor is called to free the memory of the pointers I allocated, but when it reaches "delete p_p_map[i];" in the destructor, XCode informs me that the pointer was not allocated. I am new to C++, but I feel that I pretty explicitly allocated memory to the pointers in the randomize() function.

What error am I making?

You'll benefit greatly from using `std::vector` and either `std::unique_ptr` / `boost::unique_ptr` or `std::shared_ptr` / `boost::shared_ptr` instead of managing the memory yourself. — Captain Obvlious, Nov 03 '14 at 14:39
It sounds like you're not following the [Rule of Three](http://stackoverflow.com/questions/4172722), which is easy to break if you insist on managing dynamic objects by masochistically juggling raw pointers. Use `std::vector`. — Mike Seymour, Nov 03 '14 at 14:46

score 3 · Accepted Answer · answered Nov 03 '14 at 14:38

3

You have to match delete with new and delete[] with new[]. Mixing one up with the other leads to issues. So if you do:

p_p_map = new Tile*[x];

you have to delete it like:

delete[] p_p_map;

and same with

delete[] p_p_map[i];

If you create something like:

pSomething = new Type;

then you delete it like:

delete pSomething;

answered Nov 03 '14 at 14:38

uesp

6,194
20
15

1

Also the OP's sample produces leaks in the `randomize()` function. – πάντα ῥεῖ Nov 03 '14 at 14:40
2

Indeed, `*new` is almost always a sign of a memory leak and usually should be omitted outright. Here too; use just `... = Tile( i, j, type)` – MSalters Nov 03 '14 at 14:47

score 1 · Answer 2 · answered Nov 03 '14 at 14:53

What error am I making?

A few:

First, as @uesp pointed out, you mismatch new and delete calls

Second, you are using the "memory leak operator":

p_p_map[i][j] = *new Tile(Tile::TileSize * i, Tile::TileSize * j, TileType::randomType());

The construct new Tile(...) allocates memory. Then, this memory (not stored anywhere) is dereferenced, and the result is assigned to p_p_map[i][j].

Because the pointer is not stored anywhere, it is leaked.

Third, you are not respecting RAII. While this is not technically an error in itself, the way you write the code is unsafe, and in low memory conditions, you will get UB.

For example, here's what happens if you construct a Tile instance with large values for x and y:

TileMap::TileMap(int x, int y) { // e.g. (x = 1024 * 1024, y = 1024 * 1024 * 1024)

    mapSize.x = x;
    mapSize.y = y;

    p_p_map = new Tile*[x]; // allocate 1049600 pointers block

    for(int i = 0; i < x; i++) {

        p_p_map[i] = new Tile[y]; // run out of memory (for example) half way through the loop

    }

    randomize();
}

Depending where your allocations fail, your constructor will not finish executing, meaning your TileMap instance is "half-constructed" (i.e. in an invalid state) and the destructor will not be called.

In this case, everything the class allocated is leaked, and (especially if you allocated a large size) your application is left in low memory conditions.

To fix this, make sure each pointer is managed by a different instance of a class (part of RAII). This ensures that if an allocation fails, the allocated resources are released before exitting the scope, as part of stack unwinding (as @CaptainObvlious said, use std::vector for the array and std::unique_ptr for each element).

Deleting pointers in destructor

2 Answers2