Is this the right way to delete and erase pointers to objects stored in a vector?

Question

I am not very good with STL and I saw few post similar to my requirement and got confused. So, I need some suggestion on following code.

        SomeStruct someStruct(identifier);
        std::vector<SomeStruct*>::iterator it = std::find_if(vWrapper.begin(), vWrapper.end(), SomeStruct::Find_SomeStruct(&someStruct));
        if(it != vWrapper.end()) 
        {
            ...
            delete *it;  
            it = vWrapper.erase(it);
        }

I am trying to peek into vector based on identifier and then delete pointer to object stored in vector.

I saw the post. It makes use of for loop and reassigns the iterators. Also, none of the post has used find_if() then delete and erase.

Am I doing it the right way?

Answer 1

Formally, what you're doing is undefined behavior, but practically speaking, it's fine. The undefined behavior occurs because std::vector requires that all of its contents be copiable and assignable, at all times, and delete makes the pointer value it was passed invalid, and thus uncopiable. In practice: std::vector isn't going to copy the pointer value if you erase it immediately after, and I don't know of a machine today where a deleted pointer value can't really be copied without risk. (Just don't dereference it.) If you really want to avoid the undefined behavior, you can do something like:

SomeStruct* tmp = *it;
it = vWrapper.erase(it);
delete tmp;

but frankly, I'm not sure it's worth the effort.

Answer 2

That invokes undefined behavior. For explanation, read @James Kanze's answer.

I would rather move to next question: if in the other topic, none of the answer have used std::find_if, then it is because those posts do not talk about deleting and erasing a particular element. They seem to delete all the elements, or few of them have used functor where they check which objects to delete, which is also fine.

But the major difference between your code, and their code is that your code deletes and erases at most one object, and their code could delete all objects (the one which uses functor and inside the functor it deletes the object on meeting the condition).

Well this is what I could say for a correctly written std::find_if.

However, in reality, I couldn't understand your code, especially these two lines:

//I formatted the code so that entire code is visible without 
//scrolling horizontally

SomeStruct someStruct(identifier); 
std::vector<SomeStruct*>::iterator it = std::find_if
                                   (
                                     vWrapper.begin(), 
                                     vWrapper.end(),
                                     SomeStruct::Find_SomeStruct(&identifier)
                                   );

What is the first line doing there? You declared a variable and forgot it? Well maybe, you use it somewhere else; in that case, it is okay.

But what is SomeStruct::Find_SomeStruct? Is it a nested class which can act like a functor? a static function or what? Does your compile successfully? A complete answer also depends on these questions which I posed.