std::list.erase() not calling destructor.

Started by
6 comments, last by Andrew Russell 18 years, 10 months ago
std::list<Entity*> m_EnemyList;
std::list<Entity*>::iterator m_Indx;

/*
m_EnemyListis filled with all the enemies currently in the level
The class Enemy is derived from Entity as are all objects in the world.
*/

Entity* p_Ent = new Gunship; // Gunship is derived from Enemy
delete p_Ent; // Correctly runs through all virtual destructors

Entity* p_Ent = new Gunship;
m_EnemyList.push_back(p_Ent);
m_Indx = m_EnemyList.begin();
m_EnemyList.erase(m_Indx);  //No destructors are called not even the base class Entity

Clearly this is causing memory leaks, especially considering many of my classes have dynamically allocated members. What do I need to do to get erase() to call the destructors. Am I going to have to write my own Linklist to accomplish this?
Advertisement
Quote:Original post by Grain
Clearly this is causing memory leaks, especially considering many of my classes have dynamically allocated members. What do I need to do to get erase() to call the destructors. Am I going to have to write my own Linklist to accomplish this?

Destructors are being called, but its the Entity* destructor, which doesn't actually exist. Either store the objects directly, or use a smart pointer class. Not auto_ptr, though.

CM
I believe you have to call delete on your object before you erase it. If you used a smart pointer, this would be done automatically for you.
just do:
delete *m_Indx;m_EnemyList.erase(m_Indx);


and when it's time to destroy the whole list you do:
//in a utility file not that far awaystruct delete_object{template <typename T>void operator()(T *p){ delete p;}};//delete all objectsstd::for_each( m_EnemyList.begin(), m_EnemyList.end(), delete_object());
HardDrop - hard link shell extension."Tread softly because you tread on my dreams" - Yeats
Use a boost::shared_ptr (or some other smart pointer). Convert your list from std::list<Entity*> to Grain std::list<boost::shared_ptr<Entity> >

The deconstructor for Entity* does nothing (does not delete the Entity it points to or call its deconstructor).

The deconstructor for boost::shared_ptr<Entity> will delete (and call the deconstructor of) the Entity that it points to unless it is still in use.

Much nicer.

I seriously warn against raw pointers at all (even if you use DigitalDelusion's method for removing the list). Two reasons for this -

One is that it simply takes more code to get rid of the list - this means repeated code in every place the list could disappear. In turn this means more effort and more opertunities for bugs (either if you forget to add the delete code, or you forget to make a bugfix in an occurance of your delete code).

The second reason is that if an exception comes through and unwinds it - the entire contents will be leaked (even if you're not using exceptions - it's good form to code in a way that won't die a horible death with them).
Quote:One is that it simply takes more code to get rid of the list - this means repeated code in every place the list could disappear. In turn this means more effort and more opertunities for bugs (either if you forget to add the delete code, or you forget to make a bugfix in an occurance of your delete code).

I can't think of a single time when I've had a list that could disappear in more than one place. If you have issues of ownership worked out, I don't see how this could cause problems.

Quote:The second reason is that if an exception comes through and unwinds it - the entire contents will be leaked (even if you're not using exceptions - it's good form to code in a way that won't die a horible death with them).

Unless your list is a member of a class, and the destructor contains the appropriate code to clean up the list. I think this is true of the vast majority of uses.

Personally, I use bald pointers for everything I don't need a smart pointer for. Using smart pointers for everything is just going to:
1. Hide my memory leaks, and probably make me lazy about freeing resources I've allocated.
2. Introduce unnessicary overhead.
Quote:Original post by Andrew Russell
Use a boost::shared_ptr (or some other smart pointer). Convert your list from std::list<Entity*> to Grain std::list<boost::shared_ptr<Entity> >

The deconstructor for Entity* does nothing (does not delete the Entity it points to or call its deconstructor).

The deconstructor for boost::shared_ptr<Entity> will delete (and call the deconstructor of) the Entity that it points to unless it is still in use.

Much nicer.
But if I use an Entity instead of an Entity* how am I going to store all its derived classes. Does this boost::shared_ptr allow me to do polymorphism?
Quote:Original post by Grain
But if I use an Entity instead of an Entity* how am I going to store all its derived classes. Does this boost::shared_ptr allow me to do polymorphism?

Yep. Boost's shared pointer is just like a raw pointer, only it handles deletion for you at the correct time.

If you just use Entity on its own - no you can't use polymorphisim.

So:
std::list<Entity>  // good but no polymorphisimstd::list<Entity*> // bad - requires manual tracking and deletion// good: handles deletion for you as necessary.std::list<boost::shared_ptr<Entity> >// just to make note of it - this is a different thing (so don't do it)std::list<boost::shared_ptr<Entity*> >// it is (mostly) equivelent tostd::list<Entity**>

This topic is closed to new replies.

Advertisement