How do delete items in a vector if using remove_if and items are pointers to objects?

Question

I fear that I am running into memory leak issues by doing the following:

(Sample code)

class myItem //random container stuff mostly. All primatives.
{
    int index;
    char* name;
    int val1;
    int val2;
};

class vecList
{

    vector< myitem* > *myVec;

    void delete()
    { 
        MyVec->erase(std::remove_if(myVec->begin(), MyVec->end(), IsMarkedToDelete), MyVec->end()); //leak here?
    }
};

Erase doesn't free the memory if it's a pointer, right? If I wasn't using remove_if, I could call delete on the pointer before destroying it. How would I do it in this case? Smart Pointers? I'd prefer not to re-implement everything with them and I don't really want to add the boost library.

Thanks!

well it depends on how `myitem` was created and what exactly `myitem` contains... — Smash, Nov 01 '11 at 14:37
General C++ advice: If you're using raw pointers, you're doing it wrong. (This isn't absolute, but those who know what they're doing know when to ignore this advice.) — Kerrek SB, Nov 01 '11 at 14:43
@Kerrek: With a small `nonowning_ptr` wrapper class, that is essentially a raw pointer but with a descriptive name, you can totally avoid using raw pointers. :) — Xeo, Nov 01 '11 at 14:45
@Xeo: Yay. And by the time you do that, you will probably already know how to do something like the OP asks :-) — Kerrek SB, Nov 01 '11 at 14:46
Safely destroying objects referenced by an STL container of pointers is tricky because STL iterator operations may throw exceptions: http://stackoverflow.com/questions/7902452/may-stl-iterator-methods-throw-an-exception — Raedwald, Nov 01 '11 at 14:49
you don't allocate anything dynamically, no need to worry about deleting stuff... btw, why the pointers ? Can't you just do a `vector myVec` and then in the delete function use `.` instead of `->` ? — Smash, Nov 01 '11 at 14:50
Related (but **not** duplicate) question: http://stackoverflow.com/questions/307082/cleaning-up-an-stl-list-vector-of-pointers — Raedwald, Nov 01 '11 at 14:51
@KerrekSB, @Xeo, I think "If your raw pointers own the pointee, you're doing it wrong" is more correct. I don't see `nonowning_ptr` being particularly useful. — deft_code, Nov 01 '11 at 14:52
@deft: Yeah, that description would be a better fit. And know what? That's what the `nonowning_ptr` name describes. ;) It's all about expressing your intent. — Xeo, Nov 01 '11 at 14:55
@deft_code I'd agree. It's actually rare in most applications for a pointer to own an object, and most pointers should be raw. It's only in exceptional cases where a container owns objects, but contains pointers. (I don't think I've encountered it in over 20 years of C++.) — James Kanze, Nov 01 '11 at 15:26
@Raedwald: Does that mean that I shouldn't iterate through and call delete and erase() on each item in my destructor? — Jordan, Nov 01 '11 at 15:38
@Jordan: I don't know precisely what the constraints on your code are, but if you iterate through the container you should consider the possibility that `iterator::operator++`, `iterator::operator*` or `iterator::operator->` throw an exception. — Raedwald, Nov 01 '11 at 16:00

score 9 · Accepted Answer · answered Nov 01 '11 at 14:49

9

You could just delete the item in your IsMarkedToDelete function when it returns true.

answered Nov 01 '11 at 14:49

Xeo

129,499
52
291
397

That's actually a good idea. I'll first see if I can just rework the code to not use pointers to items, but if that turns out to be impossible this might be the way to go (assuming there are no problems doing this?) – Jordan Nov 01 '11 at 15:05

score 3 · Answer 2 · answered Nov 01 '11 at 14:55

If the only pointers to the object were in the vector, then you've leaked memory as soon as you call remove_if. remove_if moves the pointers which you are keeping down, but it doesn't say anything about the values behind the iterator it returns. Thus if you have something like [a, b, c, d] (where a, b, etc. represent different pointers), then after e = remove_if( v.begin(), v.end(), matches(b) ), your vector might (and probably will) look like [a, c, d, d], with e pointing to the second d, and all trace of b lost forever.

The obvious solution would be to use shared_ptr in the vector; this would ensure that any pointer which ended up removed from the vector would be deleted. Failing that, you can use two passes: the first would be a for_each with something like:

struct DeleteIfCondition
{
    void operator()( ObjectType* &ptr ) const
    {
        if ( condition( *ptr ) ) {
            ObjectType* tmp = ptr;
            ptr = NULL;
            delete tmp;
        }
    }
};

std::for_each( v.begin(), v.end(), DeleteIfCondition() );

as the functional object, followed by:

v.erase( std::remove( v.begin(), v.end(), NULL ), v.end() );

Your functor is about what I'd use for my answer, just adding the bool return. :) — Xeo, Nov 01 '11 at 15:07

Michael Krelin - hacker · Answer 3 · 2011-11-01T14:45:12.170

1

You can use remove_if, then for_each from the return value till the end and then erase. That would, of course, make your code a bit longer. Another possibility is to store shared_ptr pointers, if your code agrees with that.

The above is a blunt lie, as Benjamin pointed out, so you're only left with "another possibility".

edited Nov 01 '11 at 14:45

answered Nov 01 '11 at 14:35

Michael Krelin - hacker

138,757
24
193
173

How would using for_each help? – Jordan Nov 01 '11 at 14:37
`std::for_each(rif=std::remove_if(...),YourVec->end(),[](myitem* i){delete i;}); std::erase(rif,YourVec->end());` — I spoke c++11 for brevity, you'd have to introduce deleter function in c++, I believe, if you don't have it handy in your project. – Michael Krelin - hacker Nov 01 '11 at 14:40
2

This assumes that remove_if does a swap, moving the removed items to the end. It makes no such promises. – Benjamin Lindley Nov 01 '11 at 14:40
So, I'm only left with using shared_ptrs? My code is pretty heavily threaded. Isn't that going to cause some issues? – Jordan Nov 01 '11 at 14:50
I don't know much about your code. The third possibility is to run through your items first to delete and nullify, for instance, items and then remove/erase nulls. – Michael Krelin - hacker Nov 01 '11 at 14:52
WOW!!, it ***is*** unspecified. That blows my mind. Surely that is an error in the standard. – deft_code Nov 01 '11 at 14:57
deft, it's not. Actually, specifying it would make `remove_if` unnecessarily inefficient. I'd rather say it was my error to assume it. – Michael Krelin - hacker Nov 01 '11 at 15:03
2

`std::partition` is available if you want to partition the sequence. – Mike Seymour Nov 01 '11 at 15:22

score 0 · Answer 4 · answered May 27 '21 at 14:10

You can use this function:

template<typename T, typename TESTFN>
void delete_if(std::vector<T*>& vec, TESTFN&& predicate)
{
   auto it = remove_if(vec.begin(), vec.end(), [&](T* item) {
      if (predicate(item)) {
         delete item;
         return true;
      }
      return false;
   });
   vec.erase(it, vec.end());
}

Ex:

delete_if(MyVec, [](T* item) { return item->index == 5; });

How do delete items in a vector if using remove_if and items are pointers to objects?

4 Answers4