Sign in to follow this  
suliman

std::list problem and crash

Recommended Posts

suliman    1652
This did work on vc++6.0 now i use vc++2005 so that might give some answers. When removing objects from this list it works like 94% of the times but otherwise it crashes with debug assertion failed expression: list iterator not incrementable
	
std::list<gameObject *>::iterator kill;
std::list<gameObject *>::iterator it;							
for ( it = myList.begin(); it != myList.end(); it++ ){			
	gameObject * uu=*it;


	//do stuff, like check removeMe if it should be removed


	//remove
	if(uu->removeMe)
	{

		kill=it;
		it++;
		www.objectList.erase(kill);
		delete uu;
	}

	}



It seemes sometimes removing an object (actually the list contains object pointers i know) from the list will corrupt it, causing a crash, but most often the list stays intact. I have the same problem with another list of similar type. Any suggestions? Erik

Share this post


Link to post
Share on other sites
MaulingMonkey    1728
www.objectList.erase(kill);

This line invalidates the iterator "kill", as well as any other iterators pointing to the element being erased (e.g. "it").

For ordered containers (such as std::list), erase(it) returns a legal, valid iterator to the next element, or end() if there isn't one. Your loop can be rewritten as:

std::list<gameObject *>::iterator it;	
for ( it = myList.begin(); it != myList.end(); ){
gameObject * uu=*it;

//do stuff, like check removeMe if it should be removed

//remove
if(uu->removeMe)
{
it = myList.erase(it);
delete uu;
}
else
{
++it;
}
}


Why your code works even 1% of the time with iterator debug checks enabled on VS2005 is beyond me, but the above should allow it to work properly. Note that I don't increment it when I erase(), since I'm already assigning it to the return, which is already the next element.

It may also be worth pointing out that the error message is complaining about the "list iterator", not the list itself. This is your first hint you might (do, in this case) have an invalid iterator as the problem, rather than a corrupted container (although using an invalid iterator could certainly cause container corruption as a side effect).

Share this post


Link to post
Share on other sites
darookie    1441
Use the SC++L:

#include <algorithm>

bool shouldRemove(gameObject * object) {

// decide whether to delete the object

if (object->removeMe) {
delete object;
return true;
}

return false;
}

// delete the objects
myList.erase(std::remove_if(myList.begin(), myList.end(), shouldRemove), myList.end());


Also try to replace your list content with boost.shared_ptr - this saves you from manually deleting the removed game objects.

HTH,
Pat.

Share this post


Link to post
Share on other sites
Zahlman    1682
Quote:
Original post by MaulingMonkey
www.objectList.erase(kill);

This line invalidates the iterator "kill", as well as any other iterators pointing to the element being erased (e.g. "it").


He presumably understands this, which is why he made 'kill' as a copy of 'it' before incrementing.

The problem is that the code, as written, increments 'it' *twice* when something is removed (once directly and once due to how the for loop is written). Presumably, this causes a detectable when you need to remove the last item ;) and also a more subtle problem the rest of the time: if two elements in a row need to be removed, the second one won't be.

You can't get around this by just not incrementing 'it' manually, because then the copying has no effect - both iterators are invalidated (since they have the same value).

Which is why, to do it yourself, OP, you take the rest of MM's advice:

Quote:
For ordered containers (such as std::list), erase(it) returns a legal, valid iterator to the next element, or end() if there isn't one. Your loop can be rewritten as:

std::list<gameObject *>::iterator it;	
for ( it = myList.begin(); it != myList.end(); ){
gameObject * uu=*it;

//do stuff, like check removeMe if it should be removed

//remove
if(uu->removeMe)
{
it = myList.erase(it);
delete uu;
}
else
{
++it;
}
}



But why struggle with that, when you can use the standard library algorithms (as suggested by darookie)? [smile]

Share this post


Link to post
Share on other sites
MaulingMonkey    1728
Quote:
Original post by Zahlman
Quote:
Original post by MaulingMonkey
www.objectList.erase(kill);

This line invalidates the iterator "kill", as well as any other iterators pointing to the element being erased (e.g. "it").


He presumably understands this, which is why he made 'kill' as a copy of 'it' before incrementing.


My brain appears to grok nonlocal initialization poorly at the moment ;_;.

In my defense, I was distracted by the shiny mismatched variable names (e.g. "www.objectList" vs "myList") and crimes against proper whitespace usage. I must have forgotten having editing out that increment before applying those changes.

I knew it seemed odd for the code to not 'asplode all the time >_>

Share this post


Link to post
Share on other sites
suliman    1652
thanks a lot guys! Now i can play my game for more then 8 seconds. I guess that explains why it only crashed sometimes (due to the fact that it worked unless it was the last object in the list)

Upgrading all old crappy lists now.

Erik

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