Reputation:
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
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
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
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
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
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