GotW #104: Smart Pointers, Part 2 (Difficulty: 5/10)

25 thoughts on “GotW #104: Smart Pointers, Part 2 (Difficulty: 5/10)”

1. 1.1 Widget can not have non-public destructor and still the callers can own – therefore delete – it.
   1.2 Callers can either own or not own the widget. To mix the two the ownership needs must be explicitly checked for like widget::is_ownable(). This is a nuisance.
   1.3 It is not possible to return from the same implementation say a static Null_Widget (for whatever reason) or a heap based widget. Special case of 1.2.

   2. unique_ptr or shared_ptr can bind to a Deleter that makes possible use of private destructors. Also, with special deleters static or heap based objects can be returned – the deleter for static would be void.

   Some people have mentioned return by reference – widget sounds polymorphic to me!

   3. auto and move semantics. (without move unique_ptr isnt returnable)

2. oh god, again.
   template left bracket widget rigth bracket
   using MyPtr = std::shared_ptr left bracket widget rigth bracket

   Now in all places we can just write MyPtr

3. By the way, for 3rd case we can use new feature
   template
   using MyPtr = std::shared_ptr;

4. 1. The raw pointer doesn’t make it clear to the caller who’s responsible for the lifetime of the widget.

   This appears to be a free function rather than a method of a class, which suggests that there is no factor entity that actually owns the widget. So either the caller is responsible to later delete the object, or there’s a static singleton factory object that owns the widget and will delete it at the end of the execution. Neither seems likely in modern code.

   2. My inclination is to return the widget by value rather than by reference. But there are various good reasons why this function might return a (raw or smart) pointer to a widget rather than the widget itself: (1) the right (derived) type can only be determined at runtime and thus it must be dynamically allocated, (2) performance concerns about returning a large object by value, (3) the ability to indicate failure by returning nullptr, (4) whether sharing of the widget is allowed or even desired.

   To accommodate those options, people seem to be assuming that the choices are (1) unique_ptr, or (2) shared_ptr. But the best choice depends a lot on circumstances that aren’t provided.

   This makes me think of pimpl. Let’s rename widget to widget_impl, and make a new widget that provides the same interface. This new widget needs a pointer to the implementation, and we can decide *there* what type of pointer it should be, what the lifetime and sharabilty rules are. Effectively, the pimpl class is a smart pointer. widget presents the same interface, even if the actual object is some derived class only known at runtime. We can return a widget efficiently, even if a widget_impl is ungainly. We could return a zombie implementation if there’s a failure. And the sharing rules can be baked into how widget handles copy and assignment.

   In the olden days (or when making a C-compatible API), we’d return some small opaque type and call it a handle. Pimpl is a handle with a lot of syntactic sugar that lets us say w.frobnicate() rather than frobnicate(h).

   3. A pimpl solution would require the all of the calling code to change (all the w->frobnicate()s must become w.frobnicates()s). Although that’s a purely mechanical change, the fact that it’s a change at all suggests that this isn’t the answer you’re looking for.

   So that brings us back to shared_ptr and unique_ptr. I supposed you could typedef a widget_ptr to one or the other, which gives you the flexibility to decide the sharing rules separately. But that still requires at least some changes in the calling code since the caller is likely calling delete on the pointer later, and that would be inappropriate if we swapping in a smart pointer.

5. Hello,

   1) as Already said, who own it ?
   2) Use a smart pointer, an unique_ptr explain you are the only owner, you can share later it if you wish
   3) The request (as I understood it) is to do not break the compatibility, not to have smart_pointer features on raw pointer

   Then my idea is to said, if you assign it to an unique_ptr you got a smart pointer
   if you assign it to a raw pointer it’s like before.

   My idea is to use a intermediate class acting as unique_ptr with a cast operator to widget* which release it.

   class widgetUniquePtr : public std::unique_ptr
   {
   public:
   widgetUniquePtr(widget *Ptr) :std::unique_ptr(Ptr)
   { }
   widgetUniquePtr(widgetUniquePtr &Ptr):std::unique_ptr(Ptr.get())
   { }
   operator widget* ()
   {
   return release();
   }
   };

   Exemple of load_widget
   widgetUniquePtr load_widget(std::string _Name)
   {
   widgetUniquePtr w(new widget(_Name));
   return w;
   }

   Exemple of client code.
   std::unique_ptr Test_unptr(load_widget(“W7”));
   widget* TestRawPtr=load_widget(“W1”);

6. Hi guys,

   From my every day live I can say that If you know that your factory function is used in millions lines of code, you should do something like this:

   typedef std::shared_ptr widget_ptr;
   widget_ptr load_widget(widget::id desired);

   And all client code should use widget_ptr instead of hardcoded shared_ptr. widget_ptr should be treated as an abstract pointer. Believe me, typedef is wonderful. It allows you to change only several lines of code, recompile and be happy :)

   Thanks

7. oops, in last sentence should be
   auto ptr = factory->create left_bracket texture right_bracket(“texture1”);

8. 1. there is nothing wrong with this code. I would say there is problem with input parameter. What if id is std::string? const widget::id& would be fine. :)

   2. it depends on resource strategy control. If widget can be shared and application is multi-threaded – it definitely needs std::shared_ptr. In single threaded application boost::intrusive_ptr is great solution.
   If widget is unique resource – your choice is std::unique_ptr.
   raw pointer is also ok. Imagine scene graph where nodes are created by factory and deleted only by their parents.

   3. I don’t think that auto should be used in cases like this:
   auto ptr = factory->create(“texture1”);
   It doesn’t say anything about ptr type. I don’t understand people who writes auto everywhere.
   BUT, if your factory has template method that receives type, then following is ok:
   auto ptr = factory->create(“texture”);

9. I think your owned_ptr might be to expensive to use compared to something that only avoid you to delete the pointed object.

10. Loïc Joly> I agree with you with the fact that there is something missing for that purpose.
    For the solution, my first idea was just the same as yours. But after thinking a bit more to this, I think it is just the wrong one.
    Instead, I think the right way would be to add a pointer wich will be more like a unique_ptr (say owner_ptr for example).
    Basically, an owner_ptr will be just like a unique_ptr. It will have a pointer to the widget and won’t share ownership with any other pointer. But you will be able to create a weak_ptr from an owner_ptr, so that you can share the widget without sharing ownership. As for the shared_ptr, it will embded a weak_reference counter, so that you will also be able to access the data after the owner_ptr have released it. The widget will only be deleted after the weak_references counter reached 0, avoiding hard to find bugs. (I thinks this can be particullary usefull in mutlithreaded scenarios, where you can easilly access a data wich have just been deleted without you being aware of that).

    Note that this has another advantage:
    If at a time, you must, for any reason, change the internal structure of your class and replace the owner_ptr by a shared_ptr, this won’t change the signature of your public method, since you always return a weak_ptr. In your case, you will have to change the ref_ptr by a weak_ptr, wich can potentially break things.

11. Vincent, returning a Widget by value won’t work if load_widget may actually return a derived class for some values of id (or if it may change to do that in future). It also will break caller code that uses operator->().

12. Loïc Joly> Nice idea. I would have call such a pointer ref_ptr (if it’s not already taken…).
    One important thing with it would be that it would forbid deleting. (without using .release() or .get() … if they are provided)

13. I think this question exemplifies a problem I have with smart pointers, as they are currently defined by the standard. When we have shared_ptr, weak_ptr or unique_ptr, we know exactly what is the semantic associated with the pointer ownership. When we see a raw pointer, it could be one of two things:
    – A non-owning pointer associated with a unique_ptr. You should not delete it. This is a fine idiom.
    – A old-time pointer, where the owning semantic is not clear. You may or may not be responsible to delete it. You should probably avoid this idiom, but most of the existing code is written that way.

    I think this could be solved by adding a new smart pointer that is to unique_ptr what weak_ptr is to shared_ptr. It would have the following semantics:
    – Default copy/move constructor and operator=
    – Default destructor (does not delete anything)
    – Can be constructed from a unique_ptr
    – The most important feature of this class is to have a name that states the intend of the code, thereby solving the ambiguity.
    – Optionally, some implementation may use the added semantic provided by this class to provide tracking so that using this pointer when the corresponding unique_ptr destructor has been called results in an easy to debug assert, instead of a hard to debug access to a deleted object. This tracking would add some non acceptable performance cost to unique_ptr, and would be controlled by some compiler flags.

    I’ve seen previous answer ask for more documentation of load_widget. I think this class would be a good compiler-enforced documentation. I don’t mind about the specific name this class may have (maybe non_owning_ptr, or observing_ptr), and I could easily write the class myself, but I’d really love it to be standardized, so that it can become a common idiom that can be taught along with unique_ptr.

14. I don’t believe that returning by copy is a good idea. The reason is that the original code returns by pointer. This suggest that, as in almost all use of factory, whatever the factory create have entity semantic, not value semantic. Returning by copy imply that the type have value semantic, but here the object identity itself is (or seems) important.

    Move semantic are for value semantic entities, like copy. Here I think we’re dealing with entity semantic.

15. 1) Assuming that the function is creating a widget resource and returning it to be owned by the caller, the problem is that return type is a raw pointer. This can result in mistakes in resource management where ownership of the widget is not correctly assumed, leading to resource leaks.

    2) load_widget should return a resource owning Widget object, copyable or move only as allowed by the resource or as required by the resource’s usage semantics. This could require significant refactoring of load_widget callers and other APIs.

    Alternatively returning a shared_ptr to be owned by the caller will probably result in the least impact on other code. shared_ptr has flexible ownership semantics and relieves users of any need for discipline in resource lifetime management. This flexibility comes at the cost of time and memory spent by the shared_ptr doing lifetime management which may not be necessary in a particular case. E.g. the shared_ptr synchronizes its reference count even if all owners guaranteed by construction to be on the same thread.

    Another alternative is unique_ptr. unique_ptr compromises between the refactoring necessary to introduce a resource owning Widget type and the performance cost of shared_ptr’s flexibility by imposing a strict ‘single owner’ requirement on the pointer. If the program already maintains a disciplined ownership graph with a single owner for each widget* then dropping in unique_ptr may be relatively easy with no performance cost and relatively few code changes. Owning pointers become unique_ptr, passing the widget to non-owners becomes ptr.get(), ownership transfers (which should be rare to nonexistent) becomes something like ptr_b = std::move(ptr_a); and of course delete or unload_widget disappears.

    3) If calling code already uses shared_ptr to capture and manage ownership of the raw pointer then changing load_widget to return a unique_ptr may require no change in to calling code. The calling code is already written to work with shared_ptr, and a shared_ptr can be constructed from a unique_ptr (so long as no deleter or allocator is being used with the shared_ptr).

16. 1) As mjklaim mentioned there are 3 possible use cases. I guess the primary problem with the code as written is that you can’t tell which one is intended. A comment would have helped, but code that is difficult to use incorrectly would be better.

    2)
    If ownership is passed to the caller, then I’d go with
    unique_ptr load_widget( widget::id desired );
    clearly states that ownership is being passed to you.

    If ownership is shared then
    shared_ptr load_widget( widget::id desired );

    I’m not as confident about the case where ownership is completely held by the factory. I don’t think returning a raw pointer is a good idea in that case either. The user could turn around and delete it. At the very least you would want
    widget* const load_widget( widget::id desired );
    Might consider
    widget& load_widget( widget::id desired );
    but you can’t return nullptr for failure in that case. In the end I think I’ll go with this though
    weak_ptr load_widget( widget::id desired );
    clearly states that the factory must own it (it must have had a shared_ptr to make the weak).
    Also what if the factory decides to destroy the object while your using it(from a different thread maybe)? weak_ptr will help with that. If nothing else you will get an error when you try to make a shared_ptr out of it(lock). The thing I don’t like about this though, is that the user could create a shared_ptr from the weak, and then hold onto it. Would be nice to be able to prevent that. maybe the * const is better because of that.
    Could probably argue that this “third” case doesn’t really exist though. Still technically “sharing” the object. Its only the length of “time” of the sharing that is different.

    3) Depends on the case again.
    If owner ship is passed to caller, the owner was probably creating a unique_ptr out of the returned ptr anyway, so no change needed. Basically the change just prevents some future bugs (caller forgetting to delete the object, exception safety issues, if they didn’t use a unique_ptr to take the return value)

    Other cases are more problematic. I don’t see anyway not modifying existing code would be possible. In these cases, the existing api would just be trusting on the caller to “do the right thing”.

17. What is most interesting, I suppose, is that already the comments diverge.

    1. Signature.

    It’s fine as far as I am concerned. You pass and idea and receive a possibly null reference to the object it references, depending on whether or not the object was found. Clean, no issue. At least in a good and modern codebase.

    In an unknown code base, it’s also unknown what the pointer is supposed to mean here. It could mean a nullable reference or that you are given ownership of the object. The function requires documentation.

    2. Recommended.

    I cannot; at least not without speculating over the ownership.

    > No ownership ? Then a simple `ptr` class, clearly indicating that there is no ownership involved and we just want a nullable reference is the best type I can think of.

    > Ownership ? Then a `unique_ptr` obviously.

    The name kinda suggests the second alternative (we could think of loading from some configuration file or something). However having worked with non-native speakers, I have come to question the choice of names more than once, so I would prefer not to have to *assume*.

    3. Going ahead.

    This question actually illuminates the previous one. It probably points the ability of a `shared_ptr` to be built directly from a `unique_ptr` and thus suggests that the function actually implied transfer of ownership to the caller.

    In this case, `unique_ptr` is fine because either the caller used `auto` to save typing, or used `unique_ptr` or `shared_ptr` and both will automatically work.

    —

    The exercise is interesting, but I am afraid to point out that the questions are not so clear cut that you may have assumed when you asked them. I would certainly feel better if they were formulated so as to take the doubt away, such as beginning `2` with *Assuming that transfer of ownership to the caller is intended*.

18. I’d vote for Vincent’s answer and move semantics, though it’s hard to know without more info/documentation – the factory method could be from an IoC container designed to manage the instance’s lifetime.

19. Nobody mentionned the possibility to return just a widget (that is having the following signature: “widget load_widget(widget::id desired);”).
    Before C++11, there were good reasons not to do that: creating a copy of the object and destroying the original could have been too expensive to consider doing that. C++11’s move semantic is an answer to the problem, however because it’s not yet automatic (I believe that if you, that means you have to go implement the move constructor for the widget class. I’m not sure what the recommendation here should be:
    1- Still don’t return objects that are expensive to copy construct (prefer unique_ptr): draw back is you have to deal with pointers, do more memory allocations…
    2- Return the object, go implement the move constructor if you measure it’s needed: draw back is I will forget to implement the move constructor, perf will be bad because there are a lot of copies of expensive objects that we didn’t bother to implement the move constructor for
    3- Return the object, always implement a move constructor for every class: draw back is the overhead of implementing and maintaining the move constructor

20. @Alf: Interesting — it didn’t occur to me that the placeholder name “widget” could connote a GUI widget. I’ve just been following Scott Meyers’ longtime convention of using “widget” and “gadget” as placeholder names for types, since those names are nicer than “foo” and “bar” but still pretty generic. I see now that both those names also could connote a GUI item, but that’s not intended here, and FWIW I plan to talk about raw pointers more in the next GotW #105…

21. The real problem is lack of documentation – period!!!!
    Solution – original engineer to be sent on programming/communication course….

22. Given:

    widget* load_widget( widget::id desired );
    

    Q1. “What’s wrong with the return type?”

    Most probably nothing.

    In the absence of documentation to the contrary, the raw pointer indicates a simple reference to a not-owned-by-you object, and that is exactly what one expects and requires for a GUI widget.

    Each GUI widget object is typically part of a list of child widgets in some parent widget, and it typically has a number of child widgets itself. Typically it is therefore destroyed by its parent, and/or it self-destructs when it is closed by the user. Destruction from client code, e.g. via a delete performed by a unique_ptr, will most often be catastrophic: a primitive delete can fail to update the parent widget’s child list, leaving dangling pointers there, and possibly it can also fail to destroy child widgets, and it can fail to properly update various kinds of external-to-the-widget state (ideally the widget class would not permit client code delete, but in practice such classes tend to permit all kinds of disastrous operations).

    However, let’s also consider the case where the naming used in the code is misleading, where this is not really a GUI widget. It could, for example, be an image frame from a web camera, and the id could serve to identify which cam. In that case the raw pointer result still indicates a simple refererence to a not-owned-by-you object, which still is most probably correct, e.g. with the OpenCV library each new frame is put in a common buffer, overwriting the earlier contents, and a non-owning pointer to that single common buffer is returned.

    On the third hand, if there are many unskilled programmers using this function, and they tend to delete pointers to widgets (with catastropic effects!), then one might consider a unique_ptr with a no-op deleter just to deal with that.

    Q2. “What is the recommended return type? Explain your answer, including any tradeoffs.”

    The generally best return type here is the raw pointer. That’s because an appropriate smart pointer type is generally not available (in particular, not available in the C++ standard library). A unique_ptr with no-op deleter would be technically correct, and could prevent unskilled programmers from doing disastrous deletes, but it would very misleadingly communicate ownership to competent programmers.

    So, assuming mostly competent programmers, unique_ptr or some other conventional smart pointer would be simply wrong here, communicating an incorrect idea of ownership.

    However, an unconventional special kind of smart pointer that cooperates with the pointee and whose operator-> throws an exception or asserts or aborts if the pointee has been destroyed, like my ZPtr from 2009 (actually a few years earlier, but that was the earliest public posting about it that I could find), could in principle be appropriate. But such a smart pointer – what should we call it? – is not part of the standard library, nor is there, to my knowledge, such a smart pointer in the Boost library (the weak_ptr idea is in that general direction, though), and as far as I know it is generally not available. Since it has a great many uses where it can prevent errors and provide a general safety net (I was primarily thinking of using it for file and stream objects, which can enter an error state) it might be generally available in the future, and then, with programmers generally knowing about it and understanding it, it might be preferable to the raw pointer result.

    Q3. “… What makes you so confident?”

    Well, I think mostly my confidence stems from years and years of exposing my ideas and logic to the leading worldwide experts, and having them mercilessly killing those ideas and chains of thought that did not work well. What’s left, including my own internal idea and logic filters, is bound to be of high quality (but of course not perfect: the time when one stops being wrong is also the time when one stops learning, which means that either one is dead, or one’s mind has devolved to the degree that one would perhaps rather want to be dead). Also, unsolicited recognition and thanks helps quite a bit on one’s confidence. :-)

23. 1. The return type is a raw pointer. Raw pointers don’t imply any ownership information. This induce that we don’t know what are the conditions for the returned object to be destroyed (if the function return non-null). The user of this function cannot know how to manage the object: do he have to manage it’s lifetime? or will the factory implementation take care of it? Is there any documentation about it? Do he have to look at the function implementation?
    At this point, precious work (and life) time of the user is already lost because he don’t have any obvious information about the object lifetime, that is an incredibly important matter for programming and cannot easily be ignored.
    Even having a garbage collector available suggest to the user how the object lifetime will be managed, while here, there is no way to guess at this point.

    BUT.

    If the project was written using C++11 and it’s associated specific best practice, then usually a raw pointer means that the user of this function don’t get ownership of the object. Raw pointers, in C++11 good practice context, always suggest a lack of ownership of any kind, while smart pointers specify which kind of ownership behavior is used.
    In a project essentially using C++11 features and it’s modern practices, the user could safely assume that the factory function will take care of the object lifetime and the user shouldn’t manage at all the lifetime of the object.

    This context is not common yet, for the time being, and with any project started before 2011, the user should just assume that there is no obvious ownership information provided with the return type, and will have to make sure what is the required use of the function, loosing precious time.

    2. All depends on the strategy of the factory itself. Basically you have 3 strategies:

    A. The factory is the only owner of the object lifetime: even if the user loose all references to the object, it will stay alive until the factory decide (or is ordered) to destroy this object, whatever the rule the factory follows. It also can be re-used or re-provided by the same factory function call with the same parameter. Still it depends on the factory behavior.
    In this strategy, in C++11, as explained above and assuming that the whole project follow C++11 best practices, it is “safe” to use a raw pointer to tell the (guru?) user that he just get a reference, no ownership involved. This strategy also imply that the factory could destroy the object while in use, without the references being “notified”. That said, in most of such implementation, the factory will not do so without the user itself asking for destruction, or being notified about the destruction of objects before it occurs, for example using an event system.

    B. The factory will share ownership of the object lifetime: the factory provide the object and will keep track of it until he needs to destroy it (either by request or by it’s implementation logic). However, the object shouldn’t be destroyed immediately if still in use. This this the strategy often use to share objects representing resources, like textures in a graphic application. In this context, as the verb ‘share’ suggest, a shared_ptr is mandatory.
    Having a shared pointer returned inform the user that whatever the use of the user, the object will be automatically destroyed when not used anymore. The factory could keep the object lifetime of the object, or maybe not. This information is not obvious but isn’t useful for the user either most of the time. What is useful to him is that he knows that the object lifetime will be managed for him and that until he don’t need the object, the object will live, even if it don’t live anymore in the factory.
    It also suggest that the returned object is the kind of data that can be used by different users, maybe in parallel, and that end of life of the object cannot be guessed.

    C. The factory give full ownership of the object to the user: the factory will only build the object, it will not manage it’s lifetime. It just throw the ball to the user for him to take care of it. This strategy is common when only the object creation process should be the responsibility of the factory, ownership being an other matter. In this context, a unique_ptr is the most useful returned type. unique_ptr will immediately imply that whoever call the function will immediately get the full and unique ownership of the referenced object. It also ease the management of that ownership by making it automatic: the user can drop the object whenever he wants and assume that it will be destroyed immediately.

    Sidenote: I personally don’t feel confident enough to assert what is good practice; but I think that C is the safer strategy to implement a factory function, letting the responsibility of the lifetime of the object directly to the user of the function or to another object which responsibility is precisely to manage the object lifetime for the end user.
    Either way, a unique_ptr is an excellent solution.

    All this assume that the default new and delete behaviour are used, but in the context of specific allocators, the use of smart pointers should be the same. Only the allocator type used in the smart pointer type instance should be different.

    3. Modern C++ imply that the code is written at a high level of abstraction, as much as possible. It imply that pointers use is simple and follow the most basic pointer semantic. Using auto abstract the real type, while the use of the object imply that it’s an iterator, whatever the real type is. So changing the type, in best case, should not force us to change the code that use this function.

    However, compilation might point semantic problems.

    If a unique_ptr is returned, because it corresponds to the strategy of the factory, then the user code should never try copy the pointer in any way. If it does, the changed code will not compile and the compiler will be able to point where the user code fails to follow the factory strategy. This is a good thing: it points the problem that have been totally non-obvious until now. Humans would have a hard time understanding what the heck is happening in this code where some pointers, at some points, start to try using destroyed objects. Using unique_ptr, all the failures of following the strategy of the factory are pointed and obvious.The code which intent is really to move instead of copy can be enhanced by adding a move() call instead of an ambiguous assignation. This is a formidable opportunity for fixing non-obvious bugs.

    If a share_ptr is returned, then whatever the user call should do (but use delete) will still work. At worst, the object lifetime will be extended far more than what was initially thought, but if the user code does keep the reference to the object then it certainly need it.

    In all cases, a quick search for delete calls might be a good idea, just to make sure that there is non in the user code, only in memory-management-specific part of the program.

    ——-

    Wow, interesting exercise!

24. I disagree with Xeo over #3. If the callers are written as auto foo = load_widget(…) then there have to be explicit deletes in the code and that is rather dated, not to mention oddlooking.

    I can only assume that all the callers were written as

    (shared_ptr or unique_ptr) foo = load_widget(…)

    and then the return type of load_widget could be changed to unique_ptr without risk as unique_ptr is convertible to shared_ptr.

25. 1) It’s a raw pointer that might leak if you’re not careful or pass it directly to a smart pointer. That, however, is a burden the caller shouldn’t have to carry if we can easily fix this on the callee site.

    2) As we learning in the answer to the GotW #103, the default smart pointer is unique_ptr. It can later be converted to a shared_ptr, if needed, and whatever deallocator frees the resource can be immediately supplied aswell, freeing the caller from having to do that.

    However, we know too little about widget at this point to tell if unique ownership is really what we want. Maybe we want shared ownership from the get-go, because internally the widgets are cached and already shared? We don’t know that, so that’s all I can say here

    3) I’m guessing auto is meant here, our sweet little type inferencing type specifier. The old code will automatically pick up the new return type without the coder having to worry about changing that.

Comments are closed.