CODEWITHSUNDEEP
c++c++11vectorstl
user8024280
user8024280

Reputation:

Removing elements from vector using remove_if

I am trying to remove vector elements using remove_if. But unsuccessfully. What am I doing wrong?

Here's my code:

#include <iostream>
#include <string>
#include <vector>
#include <algorithm>

void printme(std::vector<int>& a){
    for(const auto& item: a)
    std::cout << item << std::endl;
}

int main()
{
    std::vector<int> a {1, 2, 3, 4, 5, 6};
    printme(a);  
    a.erase( (std::remove_if(a.begin(), a.end(), [](const int& x){
        return x == 2;
        }), a.end()));
    printme(a);
}

My output is just:

1 2 3 4 5 6

Expected output:

1 2 3 4 5 6 1 3 4 5 6

Upvotes: 13

Views: 6444

Answers (5)

Yakk - Adam Nevraumont
Yakk - Adam Nevraumont

Reputation: 275976

Your problem is that you are doing the erase-remove idiom inline. It is very error prone.

template<class C, class F>
void erase_remove_if( C&& c, F&& f ) {
  using std::begin; using std::end;
  auto it = std::remove_if( begin(c), end(c), std::forward<F>(f) );
  c.erase( it, end(c) );
}

this little helper function does the error prone part of erase remove in isolation from other noise.

Then:

a.erase( (std::remove_if(a.begin(), a.end(), [](const int& x){
    return x == 2;
    }), a.end()));

becomes

erase_remove_if(
  a,
  [](const int& x){
    return x == 2;
  }
);

and suddenly your code works.

Now the proximate cause was you had a typo:

a.erase(
  (
    std::remove_if(
      a.begin(),
      a.end(),
      [](const int& x){
        return x == 2;
      }
    ),
    a.end()
  )
);

here I have expanded the line's structure. You can see from the above that you only passed one argument to erase; namely a.end(), because you passed it ( some remove expression, a.end() ) in brackets. This invoked the comma operator: so it ran the remove expression (moving the element 2 to the end), then discarded the returned iterator and evaluated to a.end().

We then passed a.end() to erase, which is not a valid iterator to pass to erase. So your program is ill-formed, and UB results.

That is only the proximate cause. There are many mistakes you can easily make when manually doing erase-remove; the code is fragile and full of repetition.

DRY is the principle that you want a single point of customization, and you don't want to repeat things that don't need repeating. erase_remove_if is my attempt to apply DRY to avoid exactly this kind of error.

Upvotes: 4

jfMR
jfMR

Reputation: 24798

You are using an overload of the std::vector::erase() member function that takes a single iterator as parameter. As the argument to erase() you are providing the iterator a.end(), since the following expression:

(std::remove_if(a.begin(), a.end(), [](const int& x){ return x == 2; }), a.end()))

evaluates to a.end() (i.e., because of the comma operator).

The iterator passed to the overload of erase() that takes a single iterator must be dereferenceable. However, the iterator a.end() is not dereferenceable, therefore, the call to erase() results in undefined behavior.

To use the overload that takes two iterators, remove the parenthesis surrounding the call to std::remove_if:

a.erase(std::remove_if(a.begin(), a.end(), [](const int& x){
        return x == 2;
        }), a.end());

Upvotes: 23

songyuanyao
songyuanyao

Reputation: 173044

You're adding superfluous parentheses, change it to

a.erase( std::remove_if(a.begin(), a.end(), [](const int& x){
    return x == 2;
    }), a.end());

Note that the comma operator just returns the last operand, that means you're passing a.end() to erase, which leads to UB.

Upvotes: 8

R Sahu
R Sahu

Reputation: 206747

The other answers have pointed out what the problem was. I want to say, it will be easier to notice these kinds of problems by simplifying your code.

I suggest using:

int main()
{
   std::vector<int> a {1, 2, 3, 4, 5, 6};
   printme(a);  

   auto it = std::remove_if(a.begin(), a.end(), [](const int& x){ return x == 2; });
   a.erase(it, a.end());

   printme(a);
}

Upvotes: 7

Petok Lorand
Petok Lorand

Reputation: 973

You just had too many parentheses in the function call.

a.erase(std::remove_if(a.begin(), a.end(), [](const int& x) {return x == 2;}), a.end());

Just remove one parentheses before the std::remove_if and at the end of the call.

Upvotes: 5

Related Questions