Debugging some code, I found an invalid use of an iterator. So I searched our code-base for further bad usages. I found a few of these:

it = cppContainer.erase( it   );

(Where it is a valid container iterator before this line, and cppContainer is a STL container, e.g. vector).

This sparked a discussion on whether the it was merely redundant, or whether it was post-incrementing the iterator after the call to the container .erase() made it invalid, which would be bad.

Is this form always a bug? Obviously the is unnecessary, but is it wrong?

The general consensus among the engineers was that it was potentially bad.

CodePudding user response：

or whether it was post-incrementing the iterator after the call to the container .erase() made it invalid.

Please note that the arguments are fully evaluated before the function is called. You can conceptually imagine the post-increment as below:

auto operator  (int) {
  auto cp = *this;
    *this;
  return cp;
}

As you can see, the increment is done before the erase function is called; ergo, this is not undefined, but possibly inefficient.

The only way this would be undefined is if it is already undefined on its own. For instance, it is the end iterator, or it is already invalidated, etc.

CodePudding user response：

Here are the points I see that can/should be addressed:

• it This increments the iterator and returns the old value, and is done before the erase. This is ok - unless it == end (or it is otherwise invalid) then it is UB.

• But the incrementing is at best pointless because erase on a std::vector

Invalidates iterators and references at or after the point of the erase *

• However, then right after it is overwritten so that nobody can use the now invalid iterator.

So, the increment is pointless and arguably dangerous since it could lead somebody to think that the assignment is needless, and that cppContainer.erase( it ); is how you should do it. This is made more confusing since: cppContainer.erase( it ); leads to it being invalid if cppContainer is a std::vector, while it is still good if it is a std::list.

Invalidates only the iterators and references to the erased elements.*

Ps. Just to be clear: it = cppContainer.erase( it ); is the canonical and correct way to do it across all standard library containers.

CodePudding user response：

Yes, it's undefined behavior. Erasing an element invalidates the iterator that pointed to it, and incrementing an invalid iterator is undefined behavior.