• 10
• 12
• 12
• 14
• 16

Don't understand why these two code blocks are different

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

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())
{
{
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())
{
{
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 on other sites
erase invalidates the iterator. You cannot use (increment, decrement, dereference) an invalidated iterator.

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 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 heredelete j; // This won't crash, because 'delete' ignores a null pointer, but suffice to say, it's not doing what it should

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 on other sites
Just fyi, delete will automatically handle null pointers, so you don't really need the "if(pObject != NULL)" check in SafeDelete.

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 on other sites
Quote:
 Original post by Julian90For your own sanity and everyone else's you should make SafeDelete an inline function instead of a macro.templatevoid 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 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 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.