C++ Array of pointers: delete or delete []?

Question

Cosider the following code:

class Foo
{
    Monster* monsters[6];

    Foo()
    {
        for (int i = 0; i < 6; i++)
        {
            monsters[i] = new Monster();
        }
    }

    virtual ~Foo();
}

What is the correct destructor?

this:

Foo::~Foo()
{
    delete [] monsters;
}

or this:

Foo::~Foo()
{
    for (int i = 0; i < 6; i++)
    {
        delete monsters[i];
    }
}

I currently have the uppermost constructor and everything is working okey, but of course I cannot see if it happens to be leaking...

Personally, I think the second version is much more logical considering what I am doing. Anyway, what is the "proper" way to do this?

Answer 1

delete[] monsters;

Is incorrect because monsters isn't a pointer to a dynamically allocated array, it is an array of pointers. As a class member it will be destroyed automatically when the class instance is destroyed.

Your other implementation is the correct one as the pointers in the array do point to dynamically allocated Monster objects.

Note that with your current memory allocation strategy you probably want to declare your own copy constructor and copy-assignment operator so that unintentional copying doesn't cause double deletes. (If you you want to prevent copying you could declare them as private and not actually implement them.)

Answer 2

Your second example is correct; you don't need to delete the monsters array itself, just the individual objects you created.

Answer 3

For new you should use delete. For new[] use delete[]. Your second variant is correct.

Answer 4

The second one is correct under the circumstances (well, the least wrong, anyway).

Edit: "least wrong", as in the original code shows no good reason to be using new or delete in the first place, so you should probably just use:

std::vector<Monster> monsters;

The result will be simpler code and cleaner separation of responsibilities.

Answer 5

You delete each pointer individually, and then you delete the entire array. Make sure you've defined a proper destructor for the classes being stored in the array, otherwise you cannot be sure that the objects are cleaned up properly. Be sure that all your destructors are virtual so that they behave properly when used with inheritance.

Answer 6

If you're not sure in this - or a similar - case, it's very simple to make a quick check: insert a printf() into the destructor.

I think delete[] is more C++-ish, but I also vote for one-by-one delete:

• maybe later some method should be called before destruction,
• some sleep should be inserted for debugging or other reason,
• the destruction must be performed in specific order,
• later, delete will be changed to passing stuff to a garbage collector.

Answer 7

delete[] monsters is definitely wrong. My heap debugger shows the following output:

allocated non-array memory at 0x3e38f0 (20 bytes)
allocated non-array memory at 0x3e3920 (20 bytes)
allocated non-array memory at 0x3e3950 (20 bytes)
allocated non-array memory at 0x3e3980 (20 bytes)
allocated non-array memory at 0x3e39b0 (20 bytes)
allocated non-array memory at 0x3e39e0 (20 bytes)
releasing     array memory at 0x22ff38

As you can see, you are trying to release with the wrong form of delete (non-array vs. array), and the pointer 0x22ff38 has never been returned by a call to new. The second version shows the correct output:

[allocations omitted for brevity]
releasing non-array memory at 0x3e38f0
releasing non-array memory at 0x3e3920
releasing non-array memory at 0x3e3950
releasing non-array memory at 0x3e3980
releasing non-array memory at 0x3e39b0
releasing non-array memory at 0x3e39e0

Anyway, I prefer a design where manually implementing the destructor is not necessary to begin with.

#include <array>
#include <memory>

class Foo
{
    std::array<std::shared_ptr<Monster>, 6> monsters;

    Foo()
    {
        for (int i = 0; i < 6; ++i)
        {
            monsters[i].reset(new Monster());
        }
    }

    virtual ~Foo()
    {
        // nothing to do manually
    }
};

Answer 8

It would make sens if your code was like this:

#include <iostream>

using namespace std;

class Monster
{
public:
        Monster() { cout << "Monster!" << endl; }
        virtual ~Monster() { cout << "Monster Died" << endl; }
};

int main(int argc, const char* argv[])
{
        Monster *mon = new Monster[6];

        delete [] mon;

        return 0;
}