On Sat, 05 Jan 2008 14:32:59 -0800, Jim Langston wrote:

Tadeusz B. Kopec wrote:

On Fri, 04 Jan 2008 12:33:24 -0800, Jim Langston wrote:

TBass wrote:

So I have a class:

class Client
{
unsigned int ClientID;
....
};


class MyListenSocket
{
...
AddClient( Client *pClient);
RemoveClient( unsigned int ID );
std::list<Client> m_listClients;
};


To add clients to the list, AddClient is called:

MyListenSocket::AddClient( Client *pClient ) {
m_listClients.push_back( *pClient );
}


But a client can be erased from anywhere on the list. I wrote this
function:

MyListenSocket::RemoveClient( unsigned int ID ) {
std::list<Client>::iterator it;
for( it = m_listClients.begin();
it != m_listClients.end();
++it )
{
if ( it->ClientID() == ID )
{
m_listClients.erase( it );
break;
}
}
}

The problem is that this seems to corrupt the heap in my program.
I know that the iterator is corrupted when I use the erase
command, but why would that corrupt the heap?

Is this not the way to delete an item from the middle of a list?
Should I not be using ::list for this type of purpose?

As others have stated, there are a few problems with this code, or
can be.

First, as stated, erase invalidates iterators. So you need to use
the algorithm:

for( it = m_listClients.begin(); it != m_listClients.end(); ) {
if ( it->ClientID() == ID )
{
it = m_listClients.erase( it );
break;
}
else
{
++it;
}
}

This change makes difference only if 'it' is used after the loop
and if value 'past the erased element' is appropriate there. Quite
strong assumption.

Actually, the for statement is executed until it !=
m_listClient.end(). If you don't use this algorithm but ++it in the
for statemnet, the iterator becomes invalidated in the erase
staement. ++it is incrementing an invalidated iterator. What
usually happens is that not all elements in the container are
visited, although it's undefined behavior so anything can happen.

After erasing break statement is executed, so no incrementation, just
leaving the loop. Yes, the value of iterator is invalid after that so
it shouldn't be used.

Try running this program and see what output you get.

#include <iostream>
#include <list>

main()
{
std::list<int> Data;
for ( int i = 0; i < 10; ++i )
Data.push_back(i);

for ( std::list<int>::iterator it = Data.begin(); it !=
Data.end(); ++it )
{
std::cout << *it << "\n";
}
std::cout << "\n";
for ( std::list<int>::iterator it = Data.begin(); it !=
Data.end(); ++it )
{
std::cout << *it << "\n";
if ( *it == 5 )
Data.erase( it );
}
}

For me, Microsoft Visual C++ .net 2003 under XP I get an access
violation reading location...

But to be equivalent to original code it should be
if ( *it == 5 )
{
Data.erase( it );
break;
}
and everything is fine (OK, I had to add return type to main).