possible inconsistency in std::vector while erasing an element [duplicate]

Question

while debugging a vector, I see an inconsistency. Assume the following code which tries to remove an entry from a vector which has only one element

#include <iostream>
#include <vector>
std::vector<int> v;
void myremove(int);
int main()
{
  v.push_back(10); 
  std::cout << "10 pushed back\n";
  myremove(10);
  std::cout << "done :)\n";
  return 0;
}

void myremove( int a )
{
  std::vector<int>::iterator it = v.begin();
  int counter = 0;
  for ( ; it != v.end(); it++ ) {
    std::cout << "iterating for " << counter << " times and vector size is " << v.size() << "\n";
    if ( a == (*it) ) {
      v.erase(it);
      std::cout << "removed " << a << "\n";
    }
    ++counter; 
  }
}

This is what I see in the output:

 $ g++ test.cpp 
 $ ./a.out | more
 10 pushed back
 iterating for 0 times and vector size is 1
 removed 10
 iterating for 1 times and vector size is 0
 iterating for 2 times and vector size is 0
 iterating for 3 times and vector size is 0
 iterating for 4 times and vector size is 0
 iterating for 5 times and vector size is 0
 iterating for 6 times and vector size is 0
 ....
 ....
 iterating for 33790 times and vector size is 0
 Segmentation fault

What I understand is that when the element is removed the size will become 0, however the iterator moves one step and still it tries to reach the end but he doesn't know that he has already passed the end point.

Can someone explain more what is going on and how to avoid that?

Answer 1

After the call to erase() the iterator it is invalidated:

Iterators and references to the erased elements and to the elements between them and the end of the container are invalidated.

Set it to the return value of erase() instead and only increment if no removal occured:

while (it != v.end())
{
    if ( a == (*it) )
    {
        it = v.erase(it);
        std::cout << "removed " << a << "\n";
    }
    else
    {
        ++it;
    }
}

where the return value of erase() is:

iterator following the last removed element.

Instead of hand coding a loop to erase elements you could use std::remove_if() instead:

v.erase(std::remove_if(v.begin(),
                       v.end(),
                       [](const int i) { return i == 10; }),
        v.end());

Answer 2

When you erase

v.erase(it);

your iterator is not valid anymore. You have to use the returned iterator from erase. Erase gives you an iterator pointing to the element that followed the element erased by the call. And you have to break the loop if you erased the last element before the loop increments it.

it = v.erase(it);
if(it == v.end())
    break;

Suggestion: you can go ahead and change the for loop to a while loop. And increment the iterator explicitly (i.e. only when you have not erased anything. If you have erased the iterator is kind of incremented already).

Like this

while(it != v.end()) {
    if ( a == (*it) ) 
      it = v.erase(it);
    else
      ++it; 
}