Jump to content
  • Advertisement
Sign in to follow this  
InsaneBoarder234

std::list.erase(iterator) - Why does this crash?

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

Can anyone tell me what the problem is with this code?
std::list<SomeObject> myList;
std::list<SomeObject>::iterator it;
std::list<SomeObject>::iterator it2;

for(it = myList.begin(); it != myList.end(); it++)
{
    if(someConditionIsMet())
    {
        //Store the iterator position where the condition was last met
        //(basically comparing items in the list, so this can change).
        it2 = it;
    }
}

//Erase an item from the list using the iterator it2
myList.erase(it2);

The last line of this code keeps firing a null pointer exception. My intention is basically to find the item in the list with the lowest value (un-sorted list) and then store a copy of the iterator to erase the item with afterwards.

Share this post


Link to post
Share on other sites
Advertisement
Are you checking that the condition was actually met at least once before using the iterator?

...
bool conditionWasMet = false;
for(...)
{
if(someConditionIsMet())
{
conditionWasMet = true;
...
}
}
if( conditionWasMet )
myList.erase(it2);




[EDIT] BTW - you should favour "++it" over "it++"
Both of the ++ operators call functions inside the STL. The "++it" version is faster and generally easier to implement. The "it++" version does more work behind the scenes, so you should only use it if you actually need it (which you don't).

[Edited by - Hodgman on February 3, 2008 9:35:41 PM]

Share this post


Link to post
Share on other sites
Quote:
Original post by InsaneBoarder234
Aha, I knew it was going to be something silly like that! Modified the code slightly to intialise it2 to myList.begin() and everything works fine now.

Thanks!


And what happens if the condition isn't met now? You'll just erase the first element no matter what! Initialise it2 to myList.end() and check for that it2 != end() before erasing.


std::list<SomeObject>::iterator it;
std::list<SomeObject>::iterator it2 = myList.end();

for(it = myList.begin(); it != myList.end(); it++)
{
if(someConditionIsMet())
{
//Store the iterator position where the condition was last met
//(basically comparing items in the list, so this can change).
it2 = it;
}
}

//Erase an item from the list using the iterator it2
if (it2 != myList.end()
{
myList.erase(it2);
}



Share this post


Link to post
Share on other sites
Quote:
Original post by ChaosEngine
And what happens if the condition isn't met now? You'll just erase the first element no matter what! Initialise it2 to myList.end() and check for that it2 != end() before erasing.

*** Source Snippet Removed ***


Sorry, yeah thats the intended behaviour. Thanks anyway though :)

Share this post


Link to post
Share on other sites
Quote:
[EDIT] BTW - you should favour "++it" over "it++"
Both of the ++ operators call functions inside the STL. The "++it" version is faster and generally easier to implement. The "it++" version does more work behind the scenes, so you should only use it if you actually need it (which you don't).


Most compilers will generate the same assembly in simple loops like this. Of course, you can always check the assembly to see for sure.

Share this post


Link to post
Share on other sites
Quote:
Original post by InsaneBoarder234
Quote:
Original post by ChaosEngine
And what happens if the condition isn't met now? You'll just erase the first element no matter what! Initialise it2 to myList.end() and check for that it2 != end() before erasing.

Sorry, yeah thats the intended behaviour. Thanks anyway though :)

If you intialise it2 to myList.begin(), then it will still crash at myList.erase(it2) if the list is empty!
If you're assuming that the list is never gonna be empty, I'd add an "assert(!myList.empty())" at the top of the function ;)


Quote:
Original post by PiNtoS
Most compilers will generate the same assembly in simple loops like this. Of course, you can always check the assembly to see for sure.

With optimisations turn on, yeah it probably will...

IMO it's still a good idea to get in the habit of saying "increment and return yourself" rather than "copy yourself, then increment yourself and return the copy" (unless of course the latter is the behaviour you're looking for).

Share this post


Link to post
Share on other sites
Quote:
Original post by InsaneBoarder234
The last line of this code keeps firing a null pointer exception.


No, it doesn't. C++ has no such beast.

Quote:
My intention is basically to find the item in the list with the lowest value (un-sorted list) and then store a copy of the iterator to erase the item with afterwards.



#include <algorithm>

if (!myList.empty()) { myList.erase(std::min_element(myList.begin(), myList.end())); }


Done.

Share this post


Link to post
Share on other sites
Sign in to follow this  

  • Advertisement
×

Important Information

By using GameDev.net, you agree to our community Guidelines, Terms of Use, and Privacy Policy.

Participate in the game development conversation and more when you create an account on GameDev.net!

Sign me up!