49

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?
Bret Kuhns
  • 4,034
  • 5
  • 31
  • 43
  • You're returning a unqiue_ptr to tell the client they own the pointer? This is exactly the opposite of what I would expect (since they have to explicitly take ownership of the unique pointer). – jeffknupp Jan 03 '12 at 21:54
  • 1
    You might want to use move-semantics instead (if you are able to use C++11). With this, it is up to the user to decide how to prolong the lifetime of the object created by the factory. – evnu Jan 03 '12 at 21:57
  • 1
    @evnu that's something that's done automatically for you, no? – Seth Carnegie Jan 03 '12 at 22:20
  • @SethCarnegie yes. I am wondering why bkuhns doesn't return an object of type `Foobar` to transfers ownership. – evnu Jan 03 '12 at 22:33
  • I just wanted to reiterate what's been said is some of the answers: instead of using `release()`, the client code should keep the pointer as a smart pointer, converting it to a `shared_ptr` if shared ownership is necessary or leaving it as `unique_ptr` otherwise. – bames53 Jan 04 '12 at 00:10
  • @bames53, Sorry, mentioning `release()` was out of context. I work on a fairly old project and I was trying to anticipate both the need and coworker concern about being able to handle a raw pointer out of the returned `unique_ptr`. – Bret Kuhns Jan 04 '12 at 00:58
  • @evnu, I hadn't considered returning the type directly (by "moving" it). That seems like a very efficient concept thanks to rvalues, and addresses all my concerns without needing a smart pointer. In general, though, I would prefer a consistent factory pattern to use throughout the whole application that works with polymorphism, and I don't imagine this technique would handle that. `unique_ptr`, as the answers below confirm, seems to be an acceptable approach, and would allow for polymoprhism. – Bret Kuhns Jan 04 '12 at 01:24
  • How would you handle a factory that could in normal operation fail with a move-return? (say, a parser that could fail to match a particular pattern, indicating that you should try another pattern). For such cases, it seems that unique_ptr is ideal, because the caller can test whether it was a successful match, and even if it is, can decide whether to explicitly move the pointer to a surviving object or let it die implicitly as it leaves its own scope. – matthias Jan 18 '12 at 21:29
  • You can have a look [here](http://wp.me/p2Bia3-2U) for typical usage of unique_ptr (especially with regards to ownership semantics). The article describes the factory pattern from your question as well as passing a unique_ptr to a function. – TiMoch Apr 13 '13 at 20:10

3 Answers3

59

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);
Trevor Hickey
  • 36,288
  • 32
  • 162
  • 271
Grizzly
  • 19,595
  • 4
  • 60
  • 78
  • 3
    I didn't mean for my mention of `release()` to be misleading or confusing, sorry. This new code is going into a fairly large existing application (1-2 million lines of C++) that doesn't use any smart pointers (other than COM). I know others on my team will be concerned if there's not a reasonable way to get to the raw pointer if legacy code makes it effectively "required". I'll, of course, give the proper warnings and recommend sticking to unique_ptr/shared_ptr when possible. I really like your typedef idea, which sets your answer apart from James McNellis'. Thanks for the insights. – Bret Kuhns Jan 04 '12 at 13:26
17

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."

James McNellis
  • 348,265
  • 75
  • 913
  • 977
  • Indeed, but why not have it return a `std::shared_pointer<>`? It achieves the same effect but allows multiple copies of the pointer. – André Caron Jan 03 '12 at 21:59
  • 8
    @AndréCaron: Why impose the use of `std::shared_ptr` if all you want to do is transfer ownership to the caller? Let the caller give ownership of the object to `std::shared_ptr` if it wants to (or another kind of smart pointer, if the caller wants to). – James McNellis Jan 03 '12 at 22:01
  • 7
    @André Caron: The reason is basically performance. The factory can't know whether or not the client will the functionality added by `std::shared_ptr` over `std::unique_ptr`, so why sacrifice performance for some functionality, which might not be necessary. Since `shared_ptr` can be constructed and assigned to from `unique_ptr` there is really no benefit – Grizzly Jan 03 '12 at 22:11
  • The caller doesn't strictly own the pointer. The caller's `unique_ptr` object owns the pointer. :p – wilhelmtell Jan 03 '12 at 22:19
  • 1
    @AndréCaron a shared_ptr can be constructed from a unique_ptr, but the reverse is generally not possible. – bames53 Jan 03 '12 at 22:32
  • @bames53: ah! That would indeed be a good reason. I think the idea is most cleanly expressed by Dietmar Kuhl's answer. Thanks! – André Caron Jan 03 '12 at 23:13
6

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.

Dietmar Kühl
  • 150,225
  • 13
  • 225
  • 380