Re: Is this the wrong way to use std::list?

From:

"Jim Langston" <tazmaster@rocketmail.com>

Newsgroups:

comp.lang.c++

Date:

Sat, 5 Jan 2008 14:32:59 -0800

Message-ID:

<GATfj.1215$dP7.354@newsfe07.lga>

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.

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

Second, the desturctor for the Client is not being called. Now, if
clients are creating using new, then you'll need to delete the
pointer with delete. This will also free the memory so you don't get
a memory leak.

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

This depends though on the ownership of the Client pointer. Who owns
the pointer? If your m_list_Clients is the only place you are
keeping this pointer, then m_listClients owns the pointer and needs
to delete it when it gets erased.

There are values not objects stored in the list.

Okay, then they don't need to be deleted. My bad on that.

--
Jim Langston
tazmaster@rocketmail.com