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

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

Answer 3

To simplify the answare let's look on the following code:

#include "stdafx.h"
#include <iostream>
using namespace std;

class A
{
private:
    int m_id;
    static int count;
public:
    A() {count++; m_id = count;}
    A(int id) { m_id = id; }
    ~A() {cout<< "Destructor A "   <<m_id<<endl; }
};

int A::count = 0;

void f1()
{   
    A* arr = new A[10];
    //delete operate only one constructor, and crash!
    delete arr;
    //delete[] arr;
}

int main()
{
    f1();
    system("PAUSE");
    return 0;
}

The output is: Destructor A 1 and then it's crashing (Expression: _BLOCK_TYPE_IS_VALID(phead- nBlockUse)).

We need to use: delete[] arr; becuse it's delete the whole array and not just one cell!

try to use delete[] arr; the output is: Destructor A 10 Destructor A 9 Destructor A 8 Destructor A 7 Destructor A 6 Destructor A 5 Destructor A 4 Destructor A 3 Destructor A 2 Destructor A 1

The same principle is for an array of pointers:

void f2()
{
    A** arr = new A*[10];
    for(int i = 0; i < 10; i++)
    {
        arr[i] = new A(i);
    }
    for(int i = 0; i < 10; i++)
    {
        delete arr[i];//delete the A object allocations.
    }

    delete[] arr;//delete the array of pointers
}

if we'll use delete arr instead of delete[] arr. it will not delete the whole pointers in the array => memory leak of pointer objects!

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

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 6

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

Answer 7

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;
}

Answer 8

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.