13

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

Improve this question
3
  • Don't you get a runtime error? Your code goes out of bounds, because vector::end() points beyond the end of your vector. Commented Jul 31, 2018 at 5:46
  • @Headcrab, when I run it on cpp.sh, there was no indication of runtime error, but when I tried in later in Visual Studio, a get runtime error which points me where the problem is Commented Jul 31, 2018 at 6:21
  • why remove_if? as long as you are gonna erase, std::find is far better in terms of efficiency. Commented Jul 31, 2018 at 13:18

5 Answers 5

Reset to default
23

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());
Improve this answer
Sign up to request clarification or add additional context in comments.

1 Comment

The lesson here is that one-liners are dangerous because they're hard to read. Separating the calls to std::remove_if and a.erase can make the code much clearer.
8

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.

Improve this answer

Comments

7

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); }
Improve this answer

Comments

5

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.

Improve this answer

Comments

4

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.

Improve this answer

Comments