remove_if removes element even when predicate returns false?

Question

I am writing an octree algorithm. Inside function I traverse octree. I get node pointer and Sphere as input. I check if node should hold sphere then I want to add it to nodes object list and remove it from its parents list. following is code

functionBody()
{
 .....

  if (!node->objectList.empty())
      node->objectList.erase(std::remove_if(node->objectList.begin(), node->objectList.end()-1 , [&t](auto& temp) { return temp == t; }));

...
}

typedef struct Sphere
{
    Sphere() = default;
    Sphere(const Vector3 centre_, const float radius_, const Material& material_) : centre(centre_), radius(radius_), material(material_)
    {
        assert(radius != 0);
        invRadius = 1.0 / radius;

        Vector3 radiusOffset = Vector3(radius);
        aabb.min = Vector3(centre - radiusOffset);
        aabb.max = Vector3(centre + radiusOffset);
    }

    bool operator==(const Sphere& rhs)
    {
        return (centre == rhs.centre) && (radius == rhs.radius);
    }

    Vector3 centre;
    float radius;
    float invRadius;
    Material material;
    AABB aabb;

}Sphere;

As you can see for Sphere I have operator== defined.

I see that remove_if is removing element even when predicate is returning false.

for example, first iteration it finds one sphere t and removed it from parents vector using remove_if. This t was present at last of vector. consider now parent remains with 3 spheres in its vector but, when I go to other child now we still try to search t in parent and remove_if still removes last entry. I don't get why?

`end()-1` is also suspicious. – Jarod42 Mar 19 '20 at 18:11 — Jarod42, Mar 19 '20 at 18:11

score 1 · Accepted Answer · answered Mar 19 '20 at 18:15

std::remove_if returns the end iterator provided when it doesn't find anything to remove. You've given node->objectList.end()-1 as the end iterator which is an iterator to the last element in node->objectList. This is what you pass to erase when you don't find t so the last element gets erased.

To fix the problem, use the overload of erase that takes a range of elements :

if (!node->objectList.empty())
{
    auto end_iter = node->objectList.end();
    auto to_remove = std::remove_if(
        node->objectList.begin(), end_iter, 
        [&t](auto& temp) { return temp == t; });

    node->objectList.erase(to_remove, end_iter);
}

Now if t isn't found erase won't do anything at all. In that case, remove_if returns end_iter and erase tried to erase the elements in the empty range defined by end_iter and itself.

I'm not sure why you were using node->objectList.end() - 1. I'm assuming it was either a mistake or a workaround for the crash that you would otherwise likely get with the previous code.

Vlad from Moscow · Answer 2 · 2020-03-19T18:28:34.670

As you are calling the method erase that accepts only one iterator (not a range of iterators) and the algorithm std::remove_if is called with the second iterator of the range of container elements specified like

node->objectList.end()-1

then even if the element is not found in the container the algorithm remove_if will return the iterator node->objectList.end()-1 that points to a valid object in the container. This object will be erased from the container.

Here is a demonstrative program that reproduces the problem.

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

int main() 
{
    std::vector<int> v = { 1, 3, 5, 7, 9 };

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

    while ( v.size() > 1 )
    {
        v.erase( std::remove_if( std::begin( v ), std::prev( std::end( v ) ),
                                 []( const auto &item )
                                 { 
                                    return item % 2 == 0; 

                                 } ) );
    }

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

    return 0;
}

Its output is

1 3 5 7 9 
1

That is none element in the vector is an even number. Nevertheless all elements except one were erased from the vector.

It seems that you are incorrectly specifying the range. It should be specified like a pair

node->objectList.begin(), node->objectList.end()

Or before erasing an element you should check whether the returned iterator is equal to node->objectList.end() - 1 (provided that you indeed want to use the range shown in your question). In this case the method erase should not be called. Or you should specify an erased range of iterators like

if (!node->objectList.empty())
      node->objectList.erase(std::remove_if(node->objectList.begin(), node->objectList.end()-1 , [&t](auto& temp) { return temp == t; }).
                             node->objectList.end()-1);

Again provided that you indeed want to use the second iterator of the range like node->objectList.end()-1 instead of node->objectList.end().

remove_if removes element even when predicate returns false?

2 Answers2