• Advertisement
Sign in to follow this  

stl::list crashing within a for loop

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

[quote name='Trienco' timestamp='1338610841' post='4945487']
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
[/quote]

Actually, I didn't. I just didn't think and blindly copied the for-loop. At least I wouldn't trust that "it" can still be incremented after erasing tmp, so the whole point is to do it first.

Fixing it now.




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
}




When already using C++11 and not worrying about the side effects of modifying the range as you iterate, you probably want to go with:

for (auto it : container) Edited by Trienco

Share this post


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

  • Advertisement