Sign in to follow this  
DarkCybo1

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

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

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