Is it safe to push_back 'dynamically allocated object' to vector?

Question

Whenever I need to add dynamically allocated object into a vector I've been doing that the following way:

class Foo { ... };

vector<Foo*> v;

v.push_back(new Foo);

// do stuff with Foo in v

// delete all Foo in v

It just worked and many others seem to do the same thing.

Today, I learned vector::push_back can throw an exception. That means the code above is not exception safe. :-( So I came up with a solution:

class Foo { ... };

vector<Foo*> v;
auto_ptr<Foo> p(new Foo);

v.push_back(p.get());
p.release();

// do stuff with Foo in v

// delete all Foo in v

But the problem is that the new way is verbose, tedious, and I see nobody's doing it. (At least not around me...)

Should I go with the new way?
Or, can I just stick with the old way?
Or, is there a better way of doing it?

Answer 1

If all you care about is exception-safety of this operation:

v.reserve(v.size()+1);  // reserve can throw, but that doesn't matter
v.push_back(new Foo);   // new can throw, that doesn't matter either.

The issue of a vector having responsibility for freeing the objects pointed to by its contents is a separate thing, I'm sure you'll get plenty of advice about that ;-)

Edit: hmm, I was going to quote the standard, but I actually can't find the necessary guarantee. What I'm looking for is that push_back will not throw unless either (a) it has to reallocate (which we know it won't because of the capacity), or (b) a constructor of T throws (which we know it won't since T is a pointer type). Sounds reasonable, but reasonable != guaranteed.

So, unless there's a beneficial answer over on this question:

Is std::vector::push_back permitted to throw for any reason other than failed reallocation or construction?

this code depends on the implementation not doing anything too "imaginative". Failing that, your solution from the question can be templated up:

template <typename T, typename Container>
void push_back_new(Container &c) {
    auto_ptr<T> p(new T);
    c.push_back(p.get());
    p.release();
}

Usage then isn't too tedious:

struct Bar : Foo { };

vector<Foo*> v;
push_back_new<Foo>(v);
push_back_new<Bar>(v);

If it's really a factory function rather than new then you could modify the template accordingly. Passing a lot of different parameter lists in different situations would be difficult, though.

Answer 2

How resilient to memory shortage is your program? If you really care about this you have to be prepared for new to throw as well. If you aren't going to handle that, I would not worry about jumping through the push_back hoops.

In the general case, if you run out of memory, the program already has likely insurmountable problems unless it's specifically designed to run forever in a constrained footprint (embedded systems) - in that case you do have to care about all of these cases.

If that applies to you, you could have a lengthy code review and retest cycle ahead of you. My guess is that you are OK to follow your team's practice here, though.

As others have pointed out, using vector to store raw pointers has its own problems, and there is a wealth of material on this site and in the other answers to direct you to a safer pattern.

Answer 3

Your new way is more exception safe but there is a reason that you don't see it done anywhere else.

A vector of pointers only owns the pointers, it doesn't express ownership of the pointed-to objects. You are effectively releasing ownership to an object that doesn't "want" ownership.

Most people will use a vector of shared_ptr to express the ownership correctly or use something like boost::ptr_vector. Either of these mean that you don't have to explicitly delete the objects whose pointers you are storing which is error prone and potentially exception 'dangerous' at other points in the program.

Edit: You still have to be very careful with insertion into ptr_vector. Unfortunately, push_back taking a raw pointer provides the strong guarantee which means that either insertion succeeds or (effectively) nothing happens, so the object passed in is neither taken over nor destroyed. The version taking a smart pointer by value is defined as calling .release() before calling the strongly guaranteed version which effectively means that it can leak.

Using a vector of shared_ptr together with make_shared is much easier to use correctly.

Answer 4

The preferred way to do this is to use a container of smart pointers, for example, a std::vector<std::shared_ptr<Foo> > or a std::vector<std::unique_ptr<Foo> > (shared_ptr can be found in Boost and C++ TR1 as well; std::unique_ptr is effectively restricted to C++0x).

Another option is to use a container that owns dynamic objects, like those containers provided by the Boost Pointer Containers library.