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

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?

Thanks in advance,
Tom Junior

On Jan 4, 2:26 pm, TBass <t...@automateddesign.comwrote:

Quote:

So I have a class:
>
class Client
{
unsigned int ClientID;
....
>
};

Lets assume that the compiler generated copy ctor for Client works for
now.

Quote:

>
class MyListenSocket
{
...
AddClient( Client *pClient);

It would be interesting to know how you are managing the pointer
above.

public:
void AddClient( const Client& client ); // much better

Quote:

RemoveClient( unsigned int ID );

private:

Quote:

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 )

void MyListenSocket::RemoveClient( const unsigned id )

Quote:

{
std::list<Client>::iterator it;

typedef std::list<Client>::iterator LIter;
LIter it;

Quote:

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?

The iterator is not corrupted, it gets invalidated. Your free store is
suffering from something else.
i'ld probably use a std::set instead of a std::list.

Quote:

>
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?
>
Thanks in advance,
Tom Junior

TBass wrote:

Quote:

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;
}
}

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.

--
Jim Langston
tazmaster@rocketmail.com

TBass <tbj@automateddesign.comwrote in news:f4b94cc5-b061-424f-bba0-
2d366f56575f@i29g2000prf.googlegroups.com:

Quote:

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 );
}

Why is this function taking a client by pointer? Why confuse the issue?
have it take the Client by const-reference.

Quote:

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?

Because you've got a problem somewhere else. Check your Client class to
make sure that it is properly copy constructable and assignable.

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

Quote:

TBass wrote:

Quote:

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

Quote:

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

--
Tadeusz B. Kopec (tkopec@NOSPAMPLEASElife.pl)
Sanity and insanity overlap a fine grey line.

Tadeusz B. Kopec wrote:

Quote:

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

Quote:

>TBass wrote:

Quote:

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

Quote:

>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

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

Quote:

Tadeusz B. Kopec wrote:

Quote:

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

Quote:

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

Quote:

>
Try running this program and see what output you get.
>
#include <iostream>
#include <list>
>
main()
{
std::list<intData;
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

Tadeusz B. Kopec wrote:

Quote:

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

Quote:

>Tadeusz B. Kopec wrote:

Quote:

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

Quote:

>>
>Try running this program and see what output you get.
>>
>#include <iostream>
>#include <list>
>>
>main()
>{
> std::list<intData;
> 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).

Dag nab it. I missread the entire code. That's what I get for trying to
respond to a post after being up for 24 hours.

--
Jim Langston
tazmaster@rocketmail.com

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

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

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

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

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

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

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

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

Similar C / C++ bytes

/bytes/development

/bytes/it

/bytes/about