Bad practice to return unique_ptr for raw pointer

Question

I've written a static factory method that returns a new Foobar object populated from another data object. I've recently been obsessed with ownership semantics and am wondering if I'm conveying the right message by having this factory method return a unique_ptr.

class Foobar {
public:
    static unique_ptr<Foobar> factory(DataObject data);
}

My intent is to tell client code that they own the pointer. Without a smart pointer, I would simply return Foobar*. I would like, however, to enforce that this memory be deleted to avoid potential bugs, so unique_ptr seemed like an appropriate solution. If the client wants to extend the lifetime of the pointer, they just call .release() once they get the unique_ptr.

Foobar* myFoo = Foobar::factory(data).release();

My question comes in two parts:

1. Does this approach convey the correct ownership semantics?
2. Is this a "bad practice" to return unique_ptr instead of a raw pointer?

Answer 1

A std::unique_ptr uniquely owns the object to which it points. It says "I own this object, and no one else does."

That is exactly what you are trying to express: you are saying "caller of this function: you are now the sole owner of this object; do with it as you please, its lifetime is your responsibility."

Answer 2

Returning a std::unique_ptr from a factory method is just fine and should be a recommended practice. The message it conveys is (IMO): You are now the sole owner of this object. Furthermore, for your convenience, the object knows how to destroy itself.

I think this is much better then returning a raw pointer (where the client has to remember how and if to dispose of this pointer).

However I do not understand your comment about releasing the pointer to extend it's lifetime. In general I rarely see any reason to call release on a smartpointer, since I think pointers should always be managed by some sort of RAII structure (just about the only situation where I call release is to put the pointer in a different managing datastructure, e.g. a unique_ptr with a different deleter, after I did something to warrant additional cleanup) .

Therefore the client can (and should) simply store the unique_ptr somewhere (such as another unique_ptr, which has been move constructed from the returned one) as long as they need the object (or a shared_ptr, if they need multiple copies of the pointer). So the clientside code should look more like this:

std::unique_ptr<FooBar> myFoo = Foobar::factory(data);
//or:
std::shared_ptr<FooBar> myFoo = Foobar::factory(data);

Personally I would also add a typedef for the returned pointer type (in this case std::unique_ptr<Foobar>) and or the used deleter (in this case std::default_deleter) to your factory object. That makes it easier if you later decide to change the allocation of your pointer(and therefore need a different method for destruction of the pointer, which will be visible as a second template parameter of std::unique_ptr). So I would do something like this:

class Foobar {
public:  
    typedef std::default_deleter<Foobar>     deleter;
    typedef std::unique_ptr<Foobar, deleter> unique_ptr;

    static unique_ptr factory(DataObject data);
}

Foobar::unique_ptr myFoo = Foobar::factory(data);
//or:
std::shared_ptr<Foobar> myFoo = Foobar::factory(data);

Answer 3

It exactly conveys the correct semantics and is the way I think all factories in C++ should work: std::unique_ptr<T> doesn't impose any kind of ownership semantics and it is extremely cheap.