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?

It's better to use a vector of `shared_ptr`, or `unique_ptr` if you can use C++11, as they allow the resources be correctly deleted in all operations (e.g. after copying the vector). — kennytm, Dec 07 '11 at 11:32
I usually recommend [using `boost::ptr_vector`](http://en.highscore.de/cpp/boost/smartpointers.html#smartpointers_pointer_container). — R. Martinho Fernandes, Dec 07 '11 at 11:37

score 5 · Accepted Answer · answered Dec 07 '11 at 11:48

5

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.

answered Dec 07 '11 at 11:48

James Kanze

150,581
18
184
329

Actually I edited my post, instead of deleting it. As the rest of the explanation in my answer seems correct, and useful for the OP (I hope) – Nawaz Dec 07 '11 at 11:54
1

@Nawaz No problem with your posting. This is a rather exceptional case where (at least IMHO), the undefined behavior is really rather pedantic. I don't think you'll ever have a problem with it in practice, and I write code like his from time to time myself. – James Kanze Dec 07 '11 at 12:21
Is it really UB to copy an invalid pointer value? Could you point me to the relevant section of the standard? – Matteo Italia Dec 07 '11 at 12:37
@MatteoItalia §3.7.4.2/4: "If the argument given to a deallocation function in the standard library is a pointer that is not the null pointer value (4.10), the deallocation function shall deallocate the storage referenced by the pointer, rendering invalid all pointers referring to any part of the deallocated storage. The effect of using an invalid pointer value (including passing it to a deallocation function) is undefined." – James Kanze Dec 07 '11 at 13:47
James, why do you say an invalid pointer is "uncopiable"? In other words, why would copying imply dereferencing? – Roman L Dec 07 '11 at 18:13
@7vies Because doing anything with an invalid value is undefined behavior. (Note that it is **not** the pointer which is invalid; it is the value it contains. As long as you don't read the pointer---there is no lvalue to rvalue conversion in standardspeak---there is no undefined behavior. But to copy it, you have to read it, and after the delete, the value the pointer contains may be a trapping value.) – James Kanze Dec 07 '11 at 19:08
James, sorry I still don't get it. If I have a pointer say `int* p = (int*)1`, would you call it "invalid"? Then I can also say `int* p2 = p;`. I copied the pointer value and there is still nothing wrong with it, right? It seems to me there shouldn't be any UB because of reading the pointer, only because of dereferencing it, i.e. `int a = *p;`. Obviously, `delete p` would be UB as well. – Roman L Dec 07 '11 at 22:41
@7vies It's clearly undefined behavior with regards to the standard. But the results of casting an int to a pointer are implementation defined (and can result in undefined behavior immediately); presumably, any such code counts on the implementation having defined some specific undefined behavior. – James Kanze Dec 08 '11 at 10:31
1

@7vies The reason lvalue to rvalue conversion of a variable containing an invalid value is undefined behavior is because all types (except char and unsigned char) may have trapping representations. In the case of pointers, one can easily imagine an implementation where 1) the hardware has separate pointer registers, which trap if an invalid pointer is read into them, and 2) the implementation forwards all malloc/free requests to the system, which invalidates the pointer. (I'm actually aware of one such implementation in C.) – James Kanze Dec 08 '11 at 10:34
James, all right, now I clearly see what you meant by "formally it's UB but practically it's fine" :) Never heard of trapping representations... Interesting, thanks! – Roman L Dec 08 '11 at 20:45

score 3 · Answer 2 · edited May 23 '17 at 12:26

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.

SomeStruct::Find_SomeStruct is a nested class which act like functor. Yes it compile successfully. Its not static. — RLT, Dec 07 '11 at 12:57

ali_bahoo · Answer 3 · 2011-12-07T12:33:48.970

1

Personally, I do not have raw pointers in a container, not to deal with problems that you ask. I do not think to choose the right way to delete them. Use a smart pointer, like std::shared_ptr, instead, and it will delete its raw pointer it owns, when the smart pointer goes out of the scope by RAII.

So instead of

std::vector<SomeStruct*> vec;

, use

std::vector<std::shared_ptr<SomeStruct>> vec;

So how about deleting an item?

vec.erase(std::remove_if( vec.begin(), vec.end(), 
  [&vec]( std::shared_ptr<SomeStruct> & item ) 
  {
    bool condition = // The condition on which the item will de deleted. e.g. item->x == 0;
    return condition;
  }
),vec.end());

edited Dec 07 '11 at 12:33

answered Dec 07 '11 at 11:33

ali_bahoo

4,732
6
41
63

An `erase` is need after `remove_if`, which only moves the items to the end and returns that iterator. – vine'th Dec 07 '11 at 11:35
`remove_if` won't erase the item (and thus shorten the vector), it just bring all non-removed ones to together. – kennytm Dec 07 '11 at 11:36
You should **never** use `remove` or `remove_if` on a container which contains (raw) pointers it owns. – James Kanze Dec 07 '11 at 11:42
@sad_man Yes, but his original posting used **raw** pointers. (If the container is to own the pointed to objects, then `shared_ptr` is the best solution. But there was nothing in his posting which supposed such.) – James Kanze Dec 07 '11 at 12:19
@JamesKanze: Yes, I should have explained the purpose of using `std::shared_ptr.` – ali_bahoo Dec 07 '11 at 12:22

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

3 Answers3