Use std::remove_if on a std::vector filled with pointers

Question

I have a little question about the std::remove_if function. I have a memory leak somewhere in my program, and I suspect the erase function to be broken somehow.

Actually, I have this in my code

std::vector<Object*> v; // Just the constructor to show you
v.erase(std::remove_if(begin(v), end(v), [Foo f](Object *o){
    return o->containsFoo(f);
}), end(v));

But after some research, is this better than the previous?

v.erase(std::remove_if(begin(v), end(v), [Foo f](Object *o){
   if(o->containsFoo(f)) {
     delete o;
     return true; 
   }
   return false;
}), end(v));

Or should I use something else?

Answer 1

You really should be using a smart pointer rather than bare Object* - either

std::vector<std::unique_ptr<int>>

or

std::vector<std::shared_ptr<int>>

whichever is appropriate. If you use naked C-style pointers, it's much too easy to miss a crucial delete (or to delete twice).

Notwithstanding, it's pretty easy to see that one approach leaks and the other does not:

#include <algorithm>
#include <vector>

int main(int argc, char **)
{
    std::vector<int*> v{ new int(1), new int(-1) };

    if (argc < 2) {
        // First version
        v.erase(std::remove_if(begin(v), end(v),
                               [](int *o){
                                   return *o < 0;
                               }),
                end(v));
    } else {
        // Second version
        v.erase(std::remove_if(begin(v), end(v),
                               [](int *o){
                                   if (*o < 0) {
                                       delete o;
                                       return true;
                                   }
                                   return false;
                               }),
                end(v));
    }

    // normal cleanup
    for (int *p: v)
        delete p;

}

I run this without an argument (invoking the first version) and then with an argument (invoking the second version). Look what happens:

g++ -std=c++11 -g -Wall -Wextra 34191606.cpp -o 34191606

---

valgrind --leak-check=full ./34191606
==16894== HEAP SUMMARY:
==16894==     in use at exit: 72,708 bytes in 2 blocks
==16894==   total heap usage: 4 allocs, 2 frees, 72,728 bytes allocated
==16894== 
==16894== 4 bytes in 1 blocks are definitely lost in loss record 1 of 2
==16894==    at 0x4C2B0E0: operator new(unsigned long) (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==16894==    by 0x400881: main (34191606.cpp:6)
==16894== 
==16894== LEAK SUMMARY:
==16894==    definitely lost: 4 bytes in 1 blocks
==16894==    indirectly lost: 0 bytes in 0 blocks
==16894==      possibly lost: 0 bytes in 0 blocks
==16894==    still reachable: 72,704 bytes in 1 blocks
==16894==         suppressed: 0 bytes in 0 blocks
==16894== Reachable blocks (those to which a pointer was found) are not shown.
==16894== ERROR SUMMARY: 1 errors from 1 contexts (suppressed: 0 from 0)

---

valgrind --leak-check=full ./34191606 -
==16895== HEAP SUMMARY:
==16895==     in use at exit: 72,704 bytes in 1 blocks
==16895==   total heap usage: 4 allocs, 3 frees, 72,728 bytes allocated
==16895== 
==16895== LEAK SUMMARY:
==16895==    definitely lost: 0 bytes in 0 blocks
==16895==    indirectly lost: 0 bytes in 0 blocks
==16895==      possibly lost: 0 bytes in 0 blocks
==16895==    still reachable: 72,704 bytes in 1 blocks
==16895==         suppressed: 0 bytes in 0 blocks
==16895== Reachable blocks (those to which a pointer was found) are not shown.
==16895== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 0 from 0)

---

Notice that in the first version, you never delete the object whose pointer you removed from the vector, and that's reported as leaked. In the second version, there's no leaked memory.

Answer 2

The lambda is better, but if I were you (and you are not prepared to use std::unique_ptr for some reason), I would do:

const auto predicate = [Foo f](Object *o){return !o->containsFoo(f);};
const auto new_end = std::partition(begin(v), end(v), predicate);
std::for_each(new_end, end(v), [](Object *o){delete o;});
v.erase(new_end,end(v));

In other words:

1. use remove_if partition to shuffle unwanted pointers to the end
2. use for_each to delete all the unwanted objects
3. use erase to get rid of the (now invalid) unwanted pointers.

The point is, that deleting pointers while things are being moved around is hairy.