C++ vector erase function not working properly

Question

I want to erase all elements with value greater 2 and less 5 here is the code :

 vector<int> myvector{3, 3, 3, 3, 3, 3, 3, 1, 2, 3, 4, 5 , 2, 3, 4, 9};
 vector<int>::iterator it;

 it = myvector.begin();
 for(int i = 0; i < myvector.size(); i++)
 {
   if(myvector[i] > 2 && myvector[i] < 5)
   {
     myvector.erase(it+i);
   }
     
 }
 for(int i = 0; i < myvector.size(); i++)
 {
   cout << ' ' << myvector[i];
     
 }

output: 3 3 3 1 2 4 5 2 4 9

Where is the problem.

Answer 1

Use the approach with applying the standard algorithm std::remove_if. For example

#include <vector>
#include <iterator>
#include <algorithm>

//...

    std::vector<int> myvector{ 3, 3, 3, 3, 3, 3, 3, 1, 2, 3, 4, 5 , 2, 3, 4, 9 };

    myvector.erase( std::remove_if( std::begin( myvector ), std::end( myvector ),
        []( const auto &item )
        {
            return 2 < item && item < 5;
        } ), std::end( myvector ) );

    for (const auto &item : myvector) std::cout << item << ' ';
    std::cout << '\n';

The output of this code snippet is

1 2 5 2 9

Using this for loop

for(int i = 0; i < myvector.size(); i++)

is incorrect. For example if you have a vector with 2 elements and in the first iteration the element with the index 0 was deleted when i will be incremented and equal to 1. So as now i equal to 1 is not less then the value returned by size() then the second element will not be deleted.

Apart from this such an approach with sequential erasing elements in a vector also is inefficient.

Answer 2

The problem is with using the erase function in a loop with iterators of that modified vector. From the cppreference page for the erase function (bolding mine):

Erases the specified elements from the container.

1. Removes the element at pos.
2. Removes the elements in the range [first, last).

Invalidates iterators and references at or after the point of the erase, including the end() iterator.

Correctly erasing elements inside a loop can be done, but it is a little tricky. The easier way is to apply the erase-remove-idiom. Or if you can use C++20, have a look at std::erase_if.