GotW #90 Solution: Factories

NOTE: Last year, I posted three new GotWs numbered #103-105. I decided leaving a gap in the numbers wasn’t best after all, so I am renumbering them to #89-91 to continue the sequence. Here is the updated version of what was GotW #104.

 

What should factory functions return, and why?

 

Problem

While spelunking through the code of a new project you recently joined, you find the following factory function declaration:

widget* load_widget( widget::id desired );

JG Question

1. What’s wrong with this return type?

Guru Questions

2. Assuming that widget is a polymorphic type, what is the recommended return type? Explain your answer, including any tradeoffs.

3. You’d like to actually change the return type to match the recommendation in #2, but at first you worry about breaking source compatibility with existing calling code; recompiling existing callers is fine, but having to go change them all is not. Then you have an “aha!” moment, realizing that this is a fairly new project and all of your calling code is written using modern C++ idioms, and you go ahead and change the return type without fear, knowing it will require few or no code changes to callers. What makes you so confident?

4. If widget is not a polymorphic type, what is the recommended return type? Explain.

 

Solution

1. What’s wrong with this return type?

First, what can we know or reasonably expect from the two-line problem description?

We’re told load_widget is a factory function. It produces an object by “loading” it in some way and then returning it to the caller. Since the return type is a pointer, the result might be null.

The caller will reasonably expect to use the object, whether by calling member functions on it or passing it to other functions or in some other way. That isn’t safe unless the caller owns the object to ensure it is alive—either the caller gets exclusive ownership, or it gets shared ownership if the factory also maintains an internal strong or weak reference.

Because the caller has or shares ownership, he has to do something when the object is no longer needed. If the ownership is exclusive, he should destroy the object somehow. Otherwise, if the ownership is shared, he should decrement some shared reference count.

Unfortunately, returning a widget* has two major problems. First, it’s unsafe by default, because the default mode of operation (i.e., when the caller writes whitespace) is to leak a widget:

// Example 1: Leak by default. Really, this is just so 20th-century...
//
widget* load_widget( widget::id desired );

:::

load_widget( some_id ); // oops

The Example 1 code compiles cleanly, runs, and (un)happily leaks the widget.

Guideline: Don’t use explicit new, delete, and owning * pointers, except in rare cases encapsulated inside the implementation of a low-level data structure.

Second, the signature conveys absolutely no information other than “a widget? sure, here you go! enjoy.” The documentation may state that (and how) the caller is to own the object, but the function declaration doesn’t—it’s either exclusive ownership or shared ownership, but which? Read and remember the function’s documentation, because the function declaration isn’t telling. The signature alone doesn’t even say whether the caller shares in ownership at all.

2. Assuming that widget is a polymorphic type, what is the recommended return type? Explain your answer, including any tradeoffs.

If widget is a type that’s intended to be used polymorphically and held by pointer or reference, the factory should normally return a unique_ptr to transfer ownership to the caller, or a shared_ptr if the factory shares ownership by retaining a strong reference inside its internal data structures.

Guideline: A factory that produces a reference type should return a unique_ptr by default, or a shared_ptr if ownership is to be shared with the factory.

This solves both problems: safety, and self-documentation.

First, consider how this immediately solves the safety problem in Example 1:

// Example 2: Clean up by default. Much better...
//
unique_ptr<widget> load_widget( widget::id desired );

:::

load_widget( some_id ); // cleans up

The Example 2 code compiles cleanly, runs, and happily cleans up the widget. But it’s not just correct by default—it’s correct by construction, because there’s no way to make a mistake that results in a leak.

Aside: Someone might say, “but can’t someone still write load_widget(some_id).release()?” Of course they can, if they’re pathological; the correct answer is, “don’t do that.” Remember, our concern is to protect against Murphy, not Machiavelli—against bugs and mistakes, not deliberate crimes—and such pathological abuses fall into the latter category. That’s no different from, and doesn’t violate type safety any more than, explicitly calling Dispose early in a C# using block or explicitly calling close early in a Java try-with-resources block.

What if the cleanup should be done by something other than a plain delete call? Easy: Just use a custom deleter. The icing on the cake is that the factory itself knows which deleter is appropriate and can state it at the time it constructs the return value; the caller doesn’t need to worry about it, especially if he takes the result using an auto variable.

Second, this is self-documenting: A function that returns a unique_ptr or a value clearly documents that it’s a pure “source” function, and one that returns a shared_ptr clearly documents that it’s returning shared ownership and/or observation.

Finally, why prefer unique_ptr by default, if you don’t need to express shared ownership? Because it’s the Right Thing To Do for both performance and correctness, as noted in GotW #89, and leaves all options open to the caller:

  • Returning unique_ptr expresses returning unique ownership, which is the norm for a pure “source” factory function.
  • unique_ptr can’t be beat in efficiency—moving one is about as cheap as moving/copying a raw pointer.
  • If the caller wants to manage the produced object’s lifetime via shared_ptr, they can easily convert to shared_ptr via an implicit move operation—no need to say std::move because the compiler already knows that the returned value is a temporary object.
  • If the caller is using any other arbitrary method of maintaining the object’s lifetime, he can convert to a custom smart pointer or other lifetime management scheme simply by calling .release(). This can be useful, and isn’t possible with shared_ptr.

Here it is in action:

// Example 2, continued
//

// Accept as a unique_ptr (by default)
auto up = load_widget(1);

// Accept as a shared_ptr (if desired)
auto sp = shared_ptr<widget>{ load_widget(2) };

// Accept as your own smart pointer (if desired)
auto msp = my::smart_ptr<widget>{ load_widget(3).release() };

Of course, if the factory retains some shared ownership or observation, whether via an internal shared_ptr or weak_ptr, return shared_ptr. The caller will be forced to continue using it as a shared_ptr, but in that case that’s appropriate.

3. […] you go ahead and change the return type without fear, knowing it will require few or no code changes to callers. What makes you so confident?

Modern portable C++ code uses unique_ptr, shared_ptr, and auto. Returning unique_ptr works with all three; returning shared_ptr works with the last two.

If the caller accepts the return value in an auto variables, such as auto w = load_widget(whatever);, then the type will just naturally be correct, normal dereferencing will just work, and the only source ripple will be if the caller tries to explicitly delete (if so, the delete line can rather appropriately be deleted) or tries to store into a non-local object of a different type.

Guideline: Prefer declaring variables using auto. It’s shorter, and helps to insulate your code from needless source ripples due to minor type changes.

Otherwise, if the caller isn’t using auto, then it’s likely already using the result to initialize a unique_ptr or shared_ptr because modern C++ calling code does not traffick in raw pointers for non-parameter variables (more on this next time). In either case, returning a unique_ptr just works: A unique_ptr can be seamlessly moved into either of those types, and if the semantics are to return shared ownership and then the caller should already be using a shared_ptr and things will again work just fine (only probably better than before, because for the original return by raw pointer to work correctly the return type was probably forced to jump through the enable_shared_from_this hoop, which isn’t needed if we just return a shared_ptr explicitly).

4. If widget is not a polymorphic type, what is the recommended return type? Explain.

If widget is not a polymorphic type, which typically means it’s a copyable value type or a move-only type, the factory should return a widget by value. But what kind of value?

In C++98, programmers would often resort to returning a large object by pointer just to avoid the penalty of copying its state:

// Example 4(a): Obsolete convention: return a * just to avoid a copy 
//
/*BAD*/ vector<gadget>* load_gadgets() {
vector<gadget>* ret = new vector<gadget>();
// ... populate *ret ...
return ret;
}

// Obsolete calling code (note: NOT exception-safe)
vector<gadget>* p = load_gadgets();
if(p) use(*p);
delete p;

This has all of the usability and fragility problems discussed in #1. Today, normally we should just return by value, because we will incur only a cheap move operation, not a deep copy, to hand the result to the caller:

// Example 4(b): Default recommendation: return the value
//
vector<gadget> load_gadgets() {
vector<gadget> ret;
// ... populate ret ...
return ret;
}

// Calling code (exception-safe)
auto v = load_gadgets();
use(v);

Most of the time, return movable objects by value. That’s all there is to it, if the only reason for the pointer on the return type was to avoid the copy.

There could be one additional reason the function might have returned a pointer, namely to return nullptr to indicate failure to produce an object. Normally it’s better throw an exception to report an error if we fail to load the widget. However, if not being able to load the widget is normal operation and should not be considered an error, return an optional<widget>, and probably make the factory noexcept if no other kinds of errors need to be reported than are communicated well by returning an empty optional<widget>.

// Example 4(c): Alternative if not returning an object is normal
//
optional<vector<gadget>> load_gadgets() noexcept {
vector<gadget> ret;
// ... populate ret ...
if( success ) // return vector (might be empty)
return move(ret); // note: move() here to avoid a silent copy
else
return {}; // not returning anything
}

// Calling code (exception-safe)
auto v = load_gadgets();
if(v) use(*v);

Guideline: A factory that produces a non-reference type should return a value by default, and throw an exception if it fails to create the object. If not creating the object can be a normal result, return an optional<> value.

Coda

By the way, see that test for if(v) in the last line of Example 4(c)? It calls a cool function of optional<T>, namely operator bool. What makes bool so cool? In part because of how many C++ features it exercises. Here is its declaration… just think of what this lets you safely do, including at compile time! Enjoy.

constexpr explicit optional<T>::operator bool() const noexcept;

Acknowledgments

Thanks in particular to the following for their feedback to improve this article: Johannes Schaub, Leo, Vincent Jacquet.

29 thoughts on “GotW #90 Solution: Factories

  1. Repeating my comment I also made on the #89 solution: BTW, the reason I’m posting #89 to #91 in faster succession is because we already saw versions of these exactly one year ago, as then-numbered #103 to #105, so some of the comment discussion about them would be duplicated and I also wanted to get more quickly to the solution for the third which I didn’t post last year. I think you’ll see that all three have been considerably updated since the initial three questions and two solutions I posted a year ago — and not just because of make_unique, and its corollary don’t-write-new, but also some other C++14-isms including the brand-new optional which, like make_unique, has only been in the draft standard since last month.

  2. “In C++98, programmers would often resort to returning a large object by pointer just to avoid the penalty of copying its state:”
    Programmers would also often resort to creating an object and then passing it by reference to copy its state. I think the solution should also be compared with that technique.

  3. @tangzhnju: If you write optional(), you know statically that the object you created is empty. So operator bool on it is a compile time constant.
    constexpr functions don’t mean “I always return a constant expression”, they mean “if my arguments are constants, so is my result”. If the value of an optional is known at compile time, so is its empty/non-empty status.

  4. When the return type of the function is “optional” and you are returning a local variable of type “T”, NRVO doesn’t apply. I think that means you should explicitly move “vector” it in that case, because the implicit move-treatment is based on the possibility of being able to do NRVO, or has that rule changed for C++14?

  5. @Sebastian Redl: Thanks for explaining constexpr to me which I did’nt quite understand before.

  6. In question 4 I think you should be talking about vector. It makes it irrelevant to compare it with the polymorphic type widget. By that I mean of course you can return by value a vector because it is heap based.

    But the question remains to be answer, would you return a widget by value if it is not a polymorphic type. The function should probably be changed to create instead of load because there is no failure possible when you return by value. But still let take a common class, a mammoth class, not something small like a Matrix4x4, but something with bad design that does too much things and has plenty of states, bools, floats,….. I would still think twice to return that by value. It might not be call often in that case well you need to decide if you pay the price. I am just saying that this is probably more what should have been the answer to number 4 and have an other question or a note for cheap moveable type like vector.

  7. @Johannes: You are correct — here’s a case where “you never have to write ‘return move'” is not correct, when they’re different types. Fixed, thanks!

    @esgames.org: Yes, if(option) can and does use explicit operator bool. That’s considered an “explicit” request for a boolean test.

  8. This article raises two questions in my mind.
    1. Does the fact that we now have explicit cast operators mean that we no longer need the “safe bool idiom”?
    2. Perhaps the standard should include a self documenting unowned_ptr template for cases where you semantically *want* to return a raw pointer.

  9. @Motti: 1. Yes. 2. What benefit is there to providing another name for * pointers? FWIW we do now teach not to use owning * pointers, but non-owning * pointers are fine — see the GotW #91 solution that will be posted tomorrow for an example.

  10. @Johannes and @Herb: This is the first time that I think I understand when to write ‘return move’. But I think that this has nothing to do with copy elision in contrast to what Johannes is saying. In the following code

    optional<vector<gadget>> load_gadgets() noexcept {
        vector<gadget> ret;
        // ... populate ret ...
        return move(ret);   // important: move() here to avoid a silent copy
    }
    

    ret is implicitely converted to optional<vector> by one of the following converting constructors of optional: constexpr optional(const T&) or constexpr optional(T&&) depending whether you write return ret; (for the former) or return move(ret); (for the latter). The result of this coversion is a temporary object, which is copied/moved into the function’s return value. An implementation is allowed to elide this last copy/move. Please correct me if I got it wrong.

  11. I wonder why we should returning optional<vector>. If no widgets can be loading what’s the benefit compared to returning an empty vector. This way I do not introduce a special case.
    Also, when loading a single gadget, returning optional is only an option (pun intended). I could also return a default gadget or an error gadget.

  12. @Marcel: Yes.

    @Vincent: Null can be different from empty because empty may be a valid value. A classic example is a nullable string, where an empty string “” is different from no string… for C-style strings, it’s the difference between

    strlen(s) == 0   // a.k.a. !strlen(s)

    and

    s == nullptr     // a.k.a. !s
  13. I understand the difference between empty and null. I am just wondering, in the context of factories, when returning a verctor of things, what is the benefit of returning a null value. How is it a better solution that returning an empty vector? Also, as currently implemented, it looks like your example 4(c) always returns a valid vector, that can be empty but not null.

    I have mitigated feelings about optional. I find that sometimes it is an easy solution but not a good solution. I would be very interrested in a post discussing when to use it and when to not use it.

  14. The advantage of having a wrapper unonwned_ptr template type is purely for self documentation. When I see T* I have to ask myself whether the person who wrote this thought about ownership and decided that a raw pointer is the correct way to go or whether (for some code bases the more probable reason) the original author was just lazy/incompetent (often the original author is myself but that doesn’t rule out incompetence…).

    After looking though the code I came to the conclusion that a raw pointer is the correct choice but tomorrow someone else will see the same code and wonder the same thing and have to duplicate the work of understanding the correct semantics for this pointer. It’s akin to why it’s better to use reinterpret_cast than a C style cast.

  15. Doesn’t the answer to #2 forego the recommended smart pointer construction through std::make_shared?

  16. @Sebastian: make_shared should be used by default when you know you’re constructing an object that will be owned by shared_ptrs. The factory doesn’t know that and so returns a unique_ptr (presumably using make_unique), which the caller can then move into a shared_ptr if that’s how he wants to manage the memory.

  17. @Vincent: That’s reasonable, I’ve added a path to the example to demonstrate returning “nothing” too so people can see what that looks like. Thanks for the suggestion.

  18. I’m not sure recommending unique_ptr et al for every pointer out there is a good idea, because it will blow up debug symbol sizes. I did a quick test and replaced the pimpl pointer of an otherwise empty class with unique_ptr – for clang 3.2 that adds 30 additional debugging symbols to the .o file, for gcc 4.8 it’s even 60 (!). I can easily imagine this kills debugging performance in larger projects …

    Any thoughts on this?

  19. @Kai: Definitely not recommending smart pointers for everything, in particular not for most parameters; see GotW #91.

    But overall tool support is an interesting question. I view it like other features such as template mangling or generating lambda function objects. The features are good for clarity and correctness, should be recommended, and will be used. Many C++ code bases have long routinely used custom nonstandard smart pointers already anyway. If sometimes a tool’s implementations has scalability trouble with a popular feature, if it causes a real problem then the tool needs to be improved. (However, for balance, there have been one or two complexity-inducing C++ features that caused tool trouble where it wasn’t fair to blame the tools, but we don’t have export any more and that was the main one.)

  20. The fact that unique_ptr forces to know the destructor of the object forces to expose the class definition at each level (which is not the case with raw pointer, neither with shared_ptr), even when passing the unique_ptr through different layers. As a consequence that each time the class definition changes, it will imply the recompilation of those intermediate layers even if they do not really use the object. This tends to brake the usage of unique_ptr with factory.
    Any comment about this ?

  21. @Christophe: I have great news for you! That’s actually not a problem at all, because unique_ptr doesn’t need to know about the destructor — only the deleter does, which can be separately compiled. In fact, unique_ptr was explicitly designed to be usable with incomplete (forward-declared types), and the standard normatively requires: “The template parameter T of unique_ptr may be an incomplete type.” (N3690, 20.9.1 [unique_ptr] /5)

  22. You missed Motti’s follow-up saying that the advantage of having a wrapper unonwned_ptr would be for self documentation(like why it’s better to use reinterpret_cast than a C style cast) and I was interested to know you opinion.
    I agree with him and dread to think of the fraction of my life that I have spent determining whether the author has passed ownership.

  23. self documentation vs auto.
    In example 2.

    unique_ptr<widget> load_widget( widget::id desired );
    

    You get self documentation—you can see in the code that you get control over widget.

    Once you use auto

    // Accept as a unique_ptr (by default)
    auto up = load_widget(1);
    

    we have to look up the signature of load_widget…it is no longer documented in the call.

    This seems to be a subtle trade off here. Any thoughts?

Comments are closed.