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?

Unkempt

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.)

Unmitigated

Can you please explain a bit why should one disable the copy constructor and the copy-assignment operator? – Endoenzyme

@Endoenzyme This is a guess, so take it for what it's worth, but I'm thinking that without the custom copy assignment/constructor the following can happen: 2 Foo objects with a monster array of pointers that point to the same monsters. If you destroy Foo1 the pointers in Foo2 will be invalid as the memory they point to has been freed by Foo1. – Damn

Answer 2

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

Soulier

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!

Dearing

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.

Shop

@Jerry I assume that Monster is a polymorphic type. In that case, some sort of pointer has to be used as the element type of the vector to enable polymorphism. – Rosaline

@FredOverflow: while it's certainly possible that he could be dealing with a polymorphic hierarchy, 1) he hasn't actually shown that, and 2) a vector will still be fine if it is. – Shop

@Jerry 1) Right 2) Absolutely – Rosaline

I am using a private array of constant length, with the members being created in the constructor and deleted in the destructor, and not being replaced in between. In such a case, I don't really see how there is an advantage to using a vector over a c array. Am I missing something? – Unkempt

@Jasper: yes. For example, what will happen if new fails (and throws an exception) when you're trying to allocate the fourth Monster in your array? – Shop

@Unkempt Is Monster itself a base class or not? – Rosaline

If Monster is a base class and it does not want to share its monsters with other Foo objects then it should be boost::ptr_vector<Monster> if not then an array of Monster objects (not monster pointers). – Ameba

@FredOverflow No, Monster is not a base class. I was using pointers because the Foo shares its Monsters with member variables (MonsterDisplayers) – Unkempt

@Unkempt Then std::vector<Monster> without any pointers is really the easiest and "most C++" solution, as suggested by Jerry. – Rosaline

This is getting way beyond the scope of the question, but in that case, would a reallocation not cause all pointers to the monsters (which were given to member variables) to become invalid? – Unkempt

@Jasper:yes -- but since you're starting from a fixed-size array, a reallocation should never happen unless you alter the whole situation so that size is dynamic. – Shop

True. I was thinking ahead - this very question came to me through "dummy content" I had created for my GUI system to display, now I was thinking about another similar case I would be facing in the real program, which will not have a fixed size (though, then again, it could probably work out just fine, as once set up, the "array" should not any new items (nor should items be erased). – Unkempt

@Jasper:yes, if there's a possibility of resizing, you'd be better off passing around an index into the vector instead of a pointer to the object in the vector. – Shop

@Jerry Coffin: but that would mean I would have to pass around the vector itself to about everywhere as well - I must say I don't quite like that either. Additionally, the MonsterDisplayers should be able to handle monsters from other sources as well (think of them as temporary monsters). Nah, that's not the solution either. Thanks for thinking about this with me, though. – Unkempt

@Jasper: if you need to have pointers/iterators to the objects remain valid across resizing of the container, a vector probably isn't the best choice of container. Assuming it stays somewhere around 6 items, a list might be a reasonable choice. – Shop

It will somewhere around 40 elements, but I guess that a list still a good option in that case. (actually, when I wrote that last comment yesterday, I thought "I guess I would need a linked list" but I didn't know if STL provided one and searched for it in the wrong way and coudn't find it... silly me) – Unkempt

@Jasper:at 40 items, I'd prefer not to use a list. Depending on the pattern of insertions (and deletions) you might be better off with an std::deque. Its rules about when iterators are invalidated are more complex, but access is almost as fast as in a vector. – Shop

Basically, there is the initialization of the data and then there's no more insertions or deletions, until destruction. However, as the only way I will be accessing the data is in a loop from n=0 to n=size, I didn't think that the performance would be all that different... Actually, as far as I know, unless you are using "at" (or an overloaded operator with the same effect) there is not much of a disadvantage to lists, is there? – Unkempt

Let me esplain a little more. First I create my Monster and put them in my "array" (I am not sure yet which container I will use) and then I start handing out Monster* to member variables (let's say myMonsterCollection and yourMonsterCollection) some of those pointers might get copied (to MonsterDisplayers) and they might move from my collection to your collection and vice versa (in actuality there are more than two locations). I run a loop over the "array" to see which monsters want to register events. The original "array" is then really only kept as the "owner". Is this a good design? – Unkempt

@Jasper: it sounds like a perfectly reasonable design -- and as long as don't hand out any pointers until after you've added all the monsters to the collection, using a vector won't be a problem; pointers/iterators into a vector can only be invalidated when you call something like push_back, or insert. – Shop

You need to do BOTH!! delete mosters[i] will delete the data being pointed to in the array. delete [] monsters will then delete the actual array! – Coleoptile

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

Rosaline

Answer 6

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

Dori

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

Nenitanenney

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.

Mount

I think doing both would be rather strange, as the array is of a constant length, so you need not delete it. Also, I did declare the destructor virtual, so that comment was pretty useless. – Unkempt