Sign in to follow this  

Safe way of deleting pointers from a list in C++?

This topic is 3933 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

Is my current method safe ?
	~CObjectFactory()
	{

		std::list< CVisEntity * >::iterator visIt = m_vpVisList.begin();

		while( visIt != m_vpVisList.end() )
		{
				
			CVisEntity * ent = *visIt;
			visIt++;
			m_vpVisList.remove( ent );
			delete ent;

		}

		std::list< CEntity * >::iterator entIt = m_vpEntList.begin();

		while( entIt != m_vpEntList.end() )
		{

			CEntity * ent = *entIt;
			entIt++;
			m_vpEntList.remove( ent );
			delete ent;		

		}

	}
Declaration of lists:

	std::list< CVisEntity * > m_vpVisList;
	std::list< CEntity * > m_vpEntList;

Share this post


Link to post
Share on other sites
It's safe, but very inefficient. Remove has to do a linear search for the item to remove. Consider using erase and assigning the return value to the iterator instead.

Share this post


Link to post
Share on other sites
A much nicer way would be to delete all the elements then clear the lists:

struct Deleter {
template< class T >
void operator()( T const * const p ) const {
delete p;
}
};

// ...
~CObjectFactory() {
// edit: since this is a destructor, you don't even need to clear()
// edit2: assuming the lists are members.
std::for_each( m_vpVisList.begin(), m_vpVisList.end(), Deleter() );
std::for_each( m_vpEntList.begin(), m_vpEntList.end(), Deleter() );
}


Or you could use smart pointers.


jfl.

Share this post


Link to post
Share on other sites
I don't see anything wrong with your code per se, but it's probably not the best way to do what you're wanting. Here's the code along with a few comments:

#if 0
~CObjectFactory()
{
std::list< CVisEntity * >::iterator visIt = m_vpVisList.begin();

while( visIt != m_vpVisList.end() )
{
CVisEntity * ent = *visIt;
visIt++;
m_vpVisList.remove( ent );
delete ent;
}

std::list< CEntity * >::iterator entIt = m_vpEntList.begin();

while( entIt != m_vpEntList.end() )
{
CEntity * ent = *entIt;
entIt++;
m_vpEntList.remove( ent );
delete ent;
}
}
#endif

// Some general problems with the above code:

// 1. Unnecessarily complex, making it more error-prone than it needs to be
// 2. Does some unnecessary work, making it less efficient than it could be
// 3. Code is copy-and-pasted, which among other things makes it more difficult to maintain

// Here's the code cleaned up a bit:

~CObjectFactory()
{
typedef std::list< CVisEntity* >::iterator visIt_t;
typedef std::list< CEntity* >::iterator entIt_t;

for (visIt_t it = m_vpVisList.begin(); it != m_vpVisList.end(); ++it) {
delete *it;
}

for (entIt_t it = m_vpEntList.begin(); it != m_vpEntList.end(); ++it) {
delete *it;
}
}

// Note that the 'for' loop is a more appropriate control structure for this
// purpose, being more concise and in a way less error-prone.

// Also note that std::list<> knows how to clean up after itself, so there's
// no need to erase its elements in the CObjectFactory destructor.

// This could be improved further by using std::for_each(), along with a
// function object to do the actual deleting. I'm not going to bother writing
// out an example - you can find the relevant information in any good reference
// on the standard library.

// A good next step would be to use 'smart' pointers (e.g. boost::shared_ptr)
// rather than 'raw' pointers, which will obviate the need for manual cleanup entirely.

// There are other improvements that could be made here, but I'm sure others
// have posted to the thread by now, so I'm going to go ahead and post this
// while it still has a chance of not being completely redundant :)








[ Edit: Bah. Too slow on the draw :| ]

Share this post


Link to post
Share on other sites
I went and used the std::for_each method, and I'm now working with a list full of 50 entities. However, I'm now getting a _BLOCK_TYPE_IS_VALID( pHead->nBlockUse ) error on the destructor call.

Share this post


Link to post
Share on other sites
Quote:
Original post by DarkCybo1
I went and used the std::for_each method, and I'm now working with a list full of 50 entities. However, I'm now getting a _BLOCK_TYPE_IS_VALID( pHead->nBlockUse ) error on the destructor call.
Can you post the revised code?

Share this post


Link to post
Share on other sites
Figured out my problem. I was sticking in referenced objects into my m_vpVisList instead of allocated pointers, which my destructor did not like.


g_ObjFactory->AddVisEntity( &ent );

Share this post


Link to post
Share on other sites

This topic is 3933 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