Sign in to follow this  

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

This topic is 4555 days old which is more than the 365 day threshold we allow for new replies. Please post a new topic.

If you intended to correct an error in the post then please contact us.

Recommended Posts

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?

Share this post


Link to post
Share on other sites
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

Share this post


Link to post
Share on other sites
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 away
struct delete_object
{
template <typename T>
void operator()(T *p){ delete p;}
};

//delete all objects
std::for_each( m_EnemyList.begin(), m_EnemyList.end(), delete_object());

Share this post


Link to post
Share on other sites
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).

Share this post


Link to post
Share on other sites
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.

Share this post


Link to post
Share on other sites
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?

Share this post


Link to post
Share on other sites
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 polymorphisim
std::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 to
std::list<Entity**>

Share this post


Link to post
Share on other sites

This topic is 4555 days old which is more than the 365 day threshold we allow for new replies. Please post a new topic.

If you intended to correct an error in the post then please contact us.

Create an account or sign in to comment

You need to be a member in order to leave a comment

Create an account

Sign up for a new account in our community. It's easy!

Register a new account

Sign in

Already have an account? Sign in here.

Sign In Now

Sign in to follow this