CODEWITHSUNDEEP
c++listpointersc++11
JordanBell
JordanBell

Reputation: 142

Deleting pointers objects in std list

I've got a std::list of Entity objects (on-screen objects). In one class, EntityContainer, I have a list of pointers to different entities. When a EntityContainer is destructed, I want all of the entities in that list to be destructed as well. How can I do this, while also avoiding the iterator errors that result in deleting members of the list?

EntityContainer::~EntityContainer()
{
    // Destroy children
    int numChildren = children.size();
    for (int i = 0; i < numChildren; i++)
    {
        Entity*& child = children.back();
        delete child;
    }
}

The above causes a null pointer access violation in std::list::clear(), which is called during the destruction of EntityContainer, as it is a member variable of that object. I would belive this is because I have deleted the objects in its list, so of course it will try to access them when deleting them. However, my problem is that if I simply leave it, and allow the list of clear() without explicitly deleting the objects within it, their destructors are never called. I can only assume this is because the list only destroys the pointers in the list, not the objects that the pointers are pointing to. That's mostly as assumption though - I may be wrong. What would you do?

Upvotes: 3

Views: 16792

Answers (4)

user3234091
user3234091

Reputation: 31

Your loop is giving an error mostly because of this

int numChildren = children.size();
for (int i = 0; i < numChildren; i++)
{
    Entity*& child = children.back(); // this always returns the last element on the last, it does not remove it.  You are trying to delete an already deleted pointer
    delete child;
}

like it has been suggested above, try something like

for(auto itr = children.begin(); itr != children.end(); ++itr)
{
    Entity* child = *itr;
    delete child;
}

Upvotes: 1

TheUndeadFish
TheUndeadFish

Reputation: 8171

Your loop isn't calling delete on every pointer in the list. It's repeatedly calling delete only on the last pointer. That's the cause of your access violation.

delete won't remove anything from a list because it doesn't know anything about it.

You need to do something like

for(auto itr = children.begin(); itr != children.end(); ++itr)
{
    Entity* child = *itr;
    delete child;
}

Or whatever your preferred loop syntax would be.

Alternatively, you could make it a list of std::unique_ptr<Entity> so that the deallocation is managed automatically when the list is cleared or entries are erased.

Upvotes: 3

Praetorian
Praetorian

Reputation: 109119

Assuming children is defined as

std::list<Entity *> children;

you can delete the elements using:

for(auto&& child : children) {
  delete child;
}
children.clear(); // not really needed since this code is in the destructor

There is no issue here with invalidating any iterators because you're not actually removing any elements from the list, only destroying the objects the list elements point to. The list will still contain the same number of elements after the for statement completes, only they'll be pointing at invalid memory at that point.

But really, don't use a container of raw pointers. Define children as

std::list<std::unique_ptr<Entity>> children;

And then you can get rid of the destructor definition.

Upvotes: 14

nneonneo
nneonneo

Reputation: 179392

std::list<Entity *> will not attempt to access any of the pointed-to objects during destruction.

Your iterated deletion code appears wrong. What you should do is just loop over the list and delete everything:

for(Entity *ptr : children) {
    delete ptr;
}

and then leave the list to clean up (deallocating the internal list of pointers).

Upvotes: 3

Related Questions