Archived

This topic is now archived and is closed to further replies.

Crispy

A question on STL list

Recommended Posts

Hi, why does my program crash if I do the following:
  
class A { ... };

std::list<A*>APool;
std::list<A*>iterator Ait;

void somefunc1()
{
  APool.push_back(new A(...));
}

void somefunc2()
{
  Ait i;

  for(i = APool.begin(); i != APool.end(); i++)
     if((*i)->TestForSomething() == false)
        APool.erase(i); //crashes here       

}
  
Thanks in advance, Crispy

Share this post


Link to post
Share on other sites
Once the condition in the if-block in your for-loop returns true, you delete the link that the iterator points to, thus invalidating the iterator. Then you try to increment the iterator. See the problem? you've just pulled the carpet out from under yourself.

Iterating deletions are extremely tricky with lists, with lots of special cases to handle. A much simpler way is to use the remove_if algorithms. It'll require you getting a handle on functors, but will do everything you want.

EDIT: tho it isn't causing your crash, EB is completely right about you needing to delete the pointers before erasing them from your list.

Don't listen to me. I've had too much coffee.

[edited by - sneftel on December 26, 2002 8:12:29 PM]

Share this post


Link to post
Share on other sites
quote:
Original post by Sneftel
Once the condition in the if-block in your for-loop returns true, you delete the link that the iterator points to, thus invalidating the iterator. Then you try to increment the iterator. See the problem? you''ve just pulled the carpet out from under yourself.


You were right here the first time. It should be:

i = APool.erase(i);

quote:

Iterating deletions are extremely tricky with lists, with lots of special cases to handle. A much simpler way is to use the remove_if algorithms. It''ll require you getting a handle on functors, but will do everything you want.


Using remove algorithms on containers of pointers is A Bad Thing. Don''t do it.

quote:

EDIT: tho it isn''t causing your crash, EB is completely right about you needing to delete the pointers before erasing them from your list.


No I''m pretty sure it IS causing the crash. Not deleting the pointers is definitely a problem (assuming they''re pointing to dynamically allocated data), but I fail to see how it would cause a crash on that particular line and not a simple memory leak (particularly since he''s using erase incorrectly.

Share this post


Link to post
Share on other sites
Not freeing memory isn''t an error, and won''t cause a crash. It is however, bad style, and will leak resources (although lets not start the debate about how the OS should clean up after the App)

Share this post


Link to post
Share on other sites
i=APool.erase(i) will not work properly in a for-loop. Immediately after the erase, the for-loop header will increment the iterator again, thus skipping an element. That skipped element will not be removed even if it matches the removal condition.

If you want to use the list:erase() return semantics, the most simple way is to use a while() loop.


i = APool.begin();

while(i != APool.end()) // you could cache the end()
{
if((*i)->TestForSomething() == false)
{
i=APool.erase(i);
}
else
{
++i;
}
}

And memory leaks never cause crashes, at least not in simple situations like this.


Don't listen to me. I've had too much coffee.

[edited by - sneftel on December 26, 2002 9:01:23 PM]

Share this post


Link to post
Share on other sites
quote:
Original post by Dobbs

You were right here the first time. It should be:

i = APool.erase(i);



How''s that? The only kind of erase() functions I see in the docs return void.

Thanks for clearing it up though, people - I''m now using a temp iterator to store the next element.

PS: in my view failing to delete the element before removing it from the list should result in memory leak at most. There are no pointers left to the data and list doesn''t require the elemnts to be sequentially placed in memory.

Crispy



Share this post


Link to post
Share on other sites