Sign in to follow this  
polaris2013

Don't understand why these two code blocks are different

Recommended Posts

m_prgSprites is a C++ Vector of <CMovable*> CMovable is a class I wrote SafeDelete is a macro I will show at the end This one crashes:
std::vector<CMovable*>::iterator j = m_prgSprites.begin();

while(j < m_prgSprites.end())
{
	if((*j)->IsDead())
	{
		m_prgSprites.erase(j);
		SafeDelete(*j);
		j = m_prgSprites.begin();
	}
}
This one doesn't:
std::vector<CMovable*>::iterator j = m_prgSprites.begin();

while(j < m_prgSprites.end())
{
	if((*j)->IsDead())
	{
		CMovable* temp = *j;
		m_prgSprites.erase(j);
		SafeDelete(temp);
		j = m_prgSprites.begin();
	}
}
SafeDelete macro:
#define SafeDelete(pObject) if(pObject != NULL) {delete pObject; pObject=NULL;}
I don't understand why the behavior is different. Any help? edit: forgot to mention, erase() does not change the value of *j, I checked in the debugger with single step.

Share this post


Link to post
Share on other sites
That's so weird, you're right. I just swapped the order of those two lines and it crashed again. Even though the values are the same! So I guess I'll delete first and erase second. Counterintuitive, but I guess it should work.

Share this post


Link to post
Share on other sites
It's not really counter-intuitive, when you examine it. If 'j' was a simple pointer, and it was erased (set to NULL/0, for example), would you expect to be able to use it to free the memory?

int* j = new int;
j = 0; // Invalidated here
delete j; // This won't crash, because 'delete' ignores a null pointer, but suffice to say, it's not doing what it should


Share this post


Link to post
Share on other sites
Also, erase returns the next valid iterator or end(), so you could use that to reset j rather than resetting it to the vector beginning.

Share this post


Link to post
Share on other sites
For your own sanity and everyone else's you should make SafeDelete an inline function instead of a macro.
template<typename T>
void SafeDelete(T*& pObject)
{
delete pObject;
pObject = NULL;
}

Share this post


Link to post
Share on other sites
Quote:
Original post by Julian90
For your own sanity and everyone else's you should make SafeDelete an inline function instead of a macro.
template<typename T>
void SafeDelete(T*& pObject)
{
delete pObject;
pObject = NULL;
}


That's not going to create an inline function, just a customized version for whatever object you are deleting. You need to put 'inline' in front of the void declaration.

Share this post


Link to post
Share on other sites
And even then I believe that the inline keyword is more of a suggestion to the compiler? Meaning that a function marked as inline may not end up being inlined. Correct me if I'm wrong.

Share this post


Link to post
Share on other sites
Aside from small linkage details, the inline keyword is mostly ignored on most compilers. However:

  1. Not trusting the compiler to inline a simple two-line function that takes its argument by reference is really a paranoid thing to do. Even code bloat is generally corrected by the main linkers.

  2. Worrying about the performance of a single call when deleting objects is also kind of silly. If you need higher performance for that specific loop (and have proved it using a profiler), there are better things you can do to increase it (such as std::remove_if).

  3. The usefulness of a SafeDelete macro or function in C++ (not C) is dubious at best. If an object owns another, then it should manifest that ownership in an exception-safe way, such as an std::auto_ptr or a boost::shared_pointer.

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