Re: Is this the wrong way to use std::list?
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).
--
Tadeusz B. Kopec (tkopec@NOSPAMPLEASElife.pl)
O'Reilly's Law of the Kitchen:
Cleanliness is next to impossible