Jump to content
  • Advertisement
Sign in to follow this  

stl::list crashing within a for loop

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

I have a stl::list within a class which I can push_back() new values into it, I can retrieve their info, but when I try to iterate through the list to find an element the application crashes.

I have narrowed my problem to this point:


list<int>::iterator it = m_vNumbers.begin();
list<int>::iterator it_end = m_vNumbers.end();
for( ; it != it_end; it++ ) // <--- CRASHING HERE!!
{
if( (*it) == obj )
{
m_vNumbers.erase( it );
}
}



The code is not complicated at all, I have one class which stores the value on the list, prints the whole list and drops away any member of the list:

#include <iostream>
#include <list>
using namespace std;
class MyClass
{
public:
void setNumber( int );
void dropNumber( int );
void print();
private:
list<int> m_vNumbers;
};



#include "MyClass.h"

void MyClass::setNumber( int obj )
{
m_vNumbers.push_back( obj );
}
void MyClass::dropNumber( int obj )
{
list<int>::iterator it = m_vNumbers.begin();
list<int>::iterator it_end = m_vNumbers.end();
for( ; it != it_end; it++ ) // <--- CRASHING HERE!!
{
if( (*it) == obj )
{
m_vNumbers.erase( it );
}
}
}

void MyClass::print()
{
cout << "All values in MyClass' list:" << endl;
list<int>::iterator it = m_vNumbers.begin();
for( ; it != m_vNumbers.end(); it++ )
{
cout << (*it) << endl;
}
}



And in main() I just do something simple as this:


int main( )
{
MyClass m_Class;
int x = 0;
int m_pObj = x;

m_Class.setNumber( m_pObj );

cout << "set value to X: ";
cin >> x;

m_Class.print(); // prints original value = 0

m_Class.dropNumber( m_pObj ); // crash

return 0;
}


and suddenly when dropping the value it crashes!!

any ideas of what could I be doing wrong?

Thanks! Edited by ???

Share this post


Link to post
Share on other sites
Advertisement
Change


m_vNumbers.erase( it );


to


it = m_vNumbers.erase( it );


You're invalidating the iterator by erasing it from the list. list<>::erase returns an iterator for this very purpose.

You will also want to change the iterator iteration logic as such:


while (it != it_end)
{
if (*it == obj)
{
it = m_vNumbers.erase( it );
}
else
it++;
}

Share this post


Link to post
Share on other sites
Here's what your program is doing:

Create [font=courier new,courier,monospace]list<int>::iterator it[/font], set it equal to [font=courier new,courier,monospace]m_vNumbers.begin()[/font].
Create [font=courier new,courier,monospace]list<int>::iterator it_end[/font], set it equal to [font=courier new,courier,monospace]m_vNumbers.end()[/font].
Check if [font=courier new,courier,monospace]it != it_end[/font]. It doesn't, so continue.
Check if [font=courier new,courier,monospace]*it[/font] is equal to [font=courier new,courier,monospace]obj[/font]. It is, so go into the if statement.
Erase [font=courier new,courier,monospace]it[/font] from [font=courier new,courier,monospace]m_vNumbers[/font] (note that this invalidates [font=courier new,courier,monospace]it[/font]!).
Now increment [font=courier new,courier,monospace]it[/font]. Crash! Why? Because you erased it and invalidated it. it doesn't exist anymore, so you can't increment it.

You can do something like this:


list<int>::iterator it = m_vNumbers.begin();
list<int>::iterator it_end = m_vNumbers.end();
while (it != it_end)
{
if( (*it) == obj )
{
m_vNumbers.erase(it++);
}
else
{
++it;
}
}


Or (in a more C++ish way):

struct ShouldRemove
{
int obj;

ShouldRemove(int o) : obj(o)
{
}

bool operator()(const std::list<int>::iterator& i) const
{
return *i == obj;
}
};

m_vNumbers.erase(std::remove_if(m_vNumbers.begin(), m_vNumbers.end(), ShouldRemove(obj)), m_vNumbers.end());


[edit]

Ninja'd, plus I wrote the wrong predicate (should be fixed). If you do the while loop, you should do it Ameise's way ([font=courier new,courier,monospace]it = m_vNumbers.erase(it)[/font]) (because I think it is clearer). Edited by Cornstalks

Share this post


Link to post
Share on other sites
i think every c++ programmer face this iterator deletion problem at least once smile.png

erase returns next iterator.


list<int>::iterator it_end = m_vNumbers.end();
for(list<int>::iterator it = m_vNumbers.begin();
; it != it_end; )
{
if( (*it) == obj )
{
it = m_vNumbers.erase( it );
}
else
it++;
}
Edited by saejox

Share this post


Link to post
Share on other sites
You might also consider the erase-remove idiom which applies to this very case:

http://en.wikipedia....se-remove_idiom

This is also a perfect candidate for c++11 lambdas.
m_vNumbers.erase(std::remove_if(m_vNumbers.begin(), m_vNumbers.end(), [obj](int num){return num==obj;}), m_vNumbers.end());
[edit] Cornstalks already has it... Edited by Madhed

Share this post


Link to post
Share on other sites
For depressing implementations where erase is defined void for some containers, a common version:


it = begin;
while (it != end)
{
tmp = it++;
if (something)
{
erase(tmp);
}
}
Edited by Trienco

Share this post


Link to post
Share on other sites

For depressing implementations where erase is defined void for some containers, a common version:


for (it = begin; it != end; ++it)
{
tmp = it++;
if (something)
{
erase(tmp);
}
}


I think you meant [font=courier new,courier,monospace]tmp = it[/font] wink.png Edited by Cornstalks

Share this post


Link to post
Share on other sites

You might also consider the erase-remove idiom which applies to this very case:

For std::list, using the remove_if() member function is more efficient than erase-remove.

Share this post


Link to post
Share on other sites
On a side note, if you're using the new C++0x standard and don't need to maintain backwards compatibility, a very convenient way to iterate through a list is by using the auto keyword, which automatically determines the type of the value in use, in this case std::list<int>::iterator:

for(auto it = list_.begin(); it != list_.end(); it++{
// Do stuff
}

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!