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!

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).

Ahhh thanks!

you're totally right!... I've changed and it now works!

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);
}
}


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]