std::list iterator usage

[gerg]

I'm having to deal with some legacy code which I think may be causeing
some problems. Can STL pro's please add comments about the correctness
of the following code:

class A
{
public:
/// ... assume constructors and such are defined
char* sz; // pointer to memory buffer
};

std::list<A> g_Alist;

/// code snippet in a cleanup routine
std::list<A>::iterator iter=NULL;
for(iter = g_Alist.begin();iter != g_Alist.end(); )
{
// seems to me that erase inside this loop could cause memory
corruption ??
g_Alist.erase(iter++);
// is iter messed up now? even if it is not, is it possible that
// it is redundant?
}
g_Alist.clean();

 

[Victor Bazarov]

I'm having to deal with some legacy code which I think may be causeing
some problems. Can STL pro's please add comments about the
correctness of the following code:

class A
{
public:
/// ... assume constructors and such are defined
char* sz; // pointer to memory buffer
Public?

};

std::list<A> g_Alist;

/// code snippet in a cleanup routine
std::list<A>::iterator iter=NULL;

There is no initialisation of a list iterator with NULL. Just declare
it inside the 'for' loop's control and initialise with 'begin()' as
usual (if that's what you need).

for(iter = g_Alist.begin();iter != g_Alist.end(); )
{
// seems to me that erase inside this loop could cause memory
corruption ??
g_Alist.erase(iter++);
// is iter messed up now? even if it is not, is it possible that
// it is redundant?
}

What's the reason to call 'erase' in a loop like that? Doesn't 'clear'
do that?

g_Alist.clean();

There is no 'clean'. I suppose you mean 'clear()'.

All in all, I think you're doing too much. If you need to remove all
elements of a list, where each element upon destruction cleans what
it should all by itself, in its own destructor, then all you need to
do is

g_Alist.clear();

That's it.

V

 

[gerg]

I agree with you that too much is being done here. I want to verify
that not only is this code doing too much but also corrupting memory.
Do you think it is doing that here?

thx,

gerg

 

[Victor Bazarov]

I agree with you that too much is being done here. I want to verify
that not only is this code doing too much but also corrupting memory.
Do you think it is doing that here?

No. You've unrolled the loop that happens behind the covers when
you call 'clear'. Then you call 'clear' on an empty list. It does
nothing.

V

 

[Thomas J. Gritzan]

I'm having to deal with some legacy code which I think may be causeing
some problems. Can STL pro's please add comments about the correctness
of the following code:

class A
{
public:
/// ... assume constructors and such are defined
char* sz; // pointer to memory buffer
};

std::list<A> g_Alist;

/// code snippet in a cleanup routine
std::list<A>::iterator iter=NULL;
for(iter = g_Alist.begin();iter != g_Alist.end(); )
{
// seems to me that erase inside this loop could cause memory
corruption ??
g_Alist.erase(iter++);
// is iter messed up now? even if it is not, is it possible that
// it is redundant?

iter is not "messed up", since you post-increment here. In a list,
erase() does only invalidates iterators to the removed elements.

What do you mean by redundant here?

}
g_Alist.clean();

Thomas

 

[gerg]

What do you mean by redundant here?

i think the loop before the clear() is not necessary.