for (i = v.begin(); i != v.end(); ) {
//...
if (erase_required) {
i = v.erase(i);
} else {
++i;
}
}
I think if you modify the collection you invalidate your iterator. You can't rely on the behavior, as you found out.
After you call erase
on an iterator into a std::map
, it is invalidated. This means that you cannot use it. Attempting to use it (e.g. by incrementing it) is invalid and can cause anything to happen (including a crash). For a std::map
, calling erase
on an iterator does not invalidate any other iterator so (for example) after this call, (so long as it
was not T2pS.end()
), it will be valid:
T2pS.erase( it++ );
Of course, if you use this approach, you won't want to unconditionally increment it
in the for loop.
For this example, though, why bother to erase in the for loop? Why not just call T2pS.clear() at the end of the loop.
On the other hand, it looks like you have a raw pointer 'on the right' of the map, but the map appears to own the pointed to object. In this case, why not make the thing on the right of the map some sort of smart pointer, such as std::tr1::shared_ptr?
[Incidentally, I don't see any template parameters to map
. Have you typedef'ed a specific instantiation of std::map
as map
in the local namespace?]
Yes, if you erase an iterator, that iterator gets a so-called singular value, which means it doesn't belong to any container anymore. You can't increment, decrement or read it out/write to it anymore. The correct way to do that loop is:
for(map<T, S*>::iterator it = T2pS.begin(); it != T2pS.end(); T2pS.erase(it++)) {
// wilhelmtell in the comments is right: no need to check for NULL.
// delete of a NULL pointer is a no-op.
if(it->second != NULL) {
delete it->second;
it->second = NULL;
}
}
For containers that could invalidate other iterators when you erase one iterator, erase
returns the next valid iterator. Then you do it with
it = T2pS.erase(it)
That's how it works for std::vector
and std::deque
, but not for std::map
or std::set
.