17

Thanks to std::make_shared, I wonder, whether the constructor for std::shared_ptr, which takes a raw pointer has any value except when interfacing with legacy / library code, e.g. when storing the output of a factory.

  • Are there other legitimate use-cases?
  • Is it reasonable advice to avoid that constructor?
  • Even to the extend of putting code checks for it in place that warn the programmer whenever it is used?
  • Should the same guidelines (whatever they are) apply to shared_ptr<T>::reset(T*)?

Regarding the code checks: I know that interfacing with legacy / library code is quite common, so automated code checks might be problematic, but in most cases I encountered so far, I'd rather use a unique_ptr anyway and I'm also not talking about a compiler warning that pops up at -Wall, but rather about a rule for static code analysis during code review.

My motivation:
It is relatively easy to say "Don't use std::shared_ptr<T>(new T(...)), always prefer std::make_shared<T>(...)" (Which I believe is correct advise?). But I'm wondering if it isn't a design smell in general, if one has to create a shared_ptr from a raw pointer, even - or especially - if the object was not just created via new, because the object should have been created either as a "shared" or "unique" object in the first place.

MikeMB
  • 20,029
  • 9
  • 57
  • 102
  • 2
    What if you want a shared-pointer-to-base-class, and you want to initialize it with a raw-pointer-to-derived-class? – The Paramagnetic Croissant Jul 24 '15 at 08:22
  • @TheParamagneticCroissant: That can be achieved with `make_shared`, no? – MikeMB Jul 24 '15 at 08:35
  • 2
    I do not believe that blindly using `make_shared` is a correct advice. Keep in mind that with make shared the allocated memory is released when the shared *and* weak counter both reach zero. This is the price of using a single allocation for the object and the control block. – sbabbi Jul 28 '15 at 10:17
  • @sbabbi True but, to be fair, having expired weak pointers stick around long enough to have this matter is itself a code smell. – David Schwartz Aug 29 '15 at 23:33

6 Answers6

14

First use case that pops to mind is when the deleter is not the default delete.

E.g. in a windows environment COM objects must some times be used, the release of these objects must be done on the object itself, via Release. Sure ATL can be used, but not everyone wants to use it.

struct ReleaseCom {
  template <class T>
  void operator() (T* p) const
  {
    p->Release();
  }
};
IComInterface* p = // co created or returned as a result
std::share_ptr<IComInterface> sp(p, ReleaseCom());

A more uncommon situation - but still valid - is when an object (handle or even raw memory) is custom allocated in a dll, OS or library and has it's own associated cleanup function that must be called (which may or may not call delete in turn). If memory allocation is involved std::allocate_shared offers more enhanced control of the allocator that is to be used without exposing a raw pointer.

My personal feeling is that given std::make_shared and std::allocate_shared, the requirement to construct a shared_ptr from raw pointers is becoming less and less. Even the cases above, can be wrapped into utility allocation and management functions, removing from the main business logic code.

Niall
  • 30,036
  • 10
  • 99
  • 142
  • Right, totally forgot about the costumizable deleter. So it would just be the single Argument constructor that possibly should be avoided. – MikeMB Jul 24 '15 at 08:28
  • @MikeMB. As a general case, yes. But that said, there are situations where the "usual" delete is required, but the code doing the allocation is not modifiable - you just get an already allocated object and need to manage it. I think the advice is prefer the `make_shared` but the constructor is still needed because the `make_shared` doesn't always cover all the use cases. – Niall Jul 24 '15 at 08:33
  • `std::allocate_shared` provides another alternative avoiding both the unprotected pointer and the default deleter. – Potatoswatter Jul 24 '15 at 08:46
  • 2
    @Potatoswatter. True, if the creation involves memory allocation, but I'm not sure that will work for "handle" type allocations (etc. file handles). – Niall Jul 24 '15 at 08:53
  • @Niall did you mean `operator(T* p)` or `operator()(T* p)`? – nonsensation Jul 24 '15 at 14:53
  • @serthy. The call operator, it's a typo – Niall Jul 24 '15 at 15:42
9

Scott Meyers lists a couple of exceptions in Effective Modern C++

The first has already been mentioned. If you need to supply a custom deleter you can't use make_shared.

The second is if you are on a system with memory concerns and you are allocating a very large object then using make_shared allocates one block for both the object and the control block that includes the weak reference count. If you have any weak_ptrs pointing to the object then the memory cannot be deallocated. On the other hand if you had not used make_shared then as soon as the last shared_ptr is deleted then the memory for the very large object can be deallocated.

Chris Drew
  • 14,926
  • 3
  • 34
  • 54
5

• Are there other legitimate use-cases?

Yes: there is a situation when a resource doesn't map to new/delete:

handle_type APIXCreateHandle(); // third party lib
void APIXDestroyHandle(handle_type h); // third party lib

In this case, you will want to use the constructor directly.

• Is it reasonable advice to avoid that constructor?

When the functionality overlaps with make_shared, yes.

• Should the same guidelines (whatever they are) apply to shared_ptr::reset(T*)?

I think they shouldn't. If you really want, you can replace the reset call with an assignment with the result of a std::make_shared call. I would prefer to see the reset call instead though - it would be more explicit in intent.

Regarding the code checks: I know that interfacing with legacy / library code is quite common

Consider wrapping the third party library into an interface that ensures the returned values are unique_ptr-wrapped. This will provide you with a point of centralization and the opportunity for other convenience/safety optimizations.

It is relatively easy to say [...] (Which I believe is correct advise?). But I'm wondering if it isn't a design smell in general

It is not design smell to use it; only to use it when std::make_shared/std::make_unique would work just as well.

Edit: to address the point of the question: you will probably not be able to add a static-analysis rule for this, unless you also add a convention/exception list to it (i.e. "except for custom deleters and third party lib API adaptation layers, make_shared and make_unique should always be used"). Such a rule would probably be skipped for some of your files.

When you do use the constructors directly though, it may be a good idea to have them in a function dedicated to just that (similar to what make_unique and make_shared do):

namespace api_x
{
    std::shared_ptr<handle_type> make_handle(); // calls APIXCreateHandle internally
                                                // also calls the constructor
                                                // to std::shared_ptr
}
utnapistim
  • 26,809
  • 3
  • 46
  • 82
3

Use std::make_shared whenever you can.

There are two reasons you may not get away with it though:

  • You want to use a custom deleter for your object. This might be the case if you interface with code that does not use RAII. Other answers provide more details.

  • You've got a custom new operator that you want to call. std::make_shared can't call it for you, because the whole point of it is to allocate memory once (though not required by the standard). See http://ideone.com/HjmFl1.

The reasoning behind std::make_shared is not that you avoid the new keyword, but that you avoid a memory allocation. A shared pointer needs to set up some control data structure. So when setting up a shared_ptr without make_shared, you have to do two allocations: One for the object, another one for the control structure. make_shared (usually) only allocates one (larger) block of memory and constructs the contained object and the control structure in place. The reason behind make_shared is (initially was) that it is more efficient in most cases, not that the syntax is prettier. This is also the reason why there was no std::make_unique in C++11. (As ChrisDrew pointed out, I suggested that std::make_unique was only added for symmetry/prettier syntax. It can help to write exception safe code in a more compact way though, see Herb Sutter's GotW #102 or this question.)

Community
  • 1
  • 1
Markus Mayr
  • 4,038
  • 1
  • 20
  • 42
  • 2
    You are right about the original reason for `make_shared` but avoiding the `new` keyword is not just about making "the syntax prettier" it also makes for more exception safe code. And _that_ is the main reason `make_unique` was included in C++14. – Chris Drew Jul 24 '15 at 09:20
3

In addition to the other answers, you cannot use make_shared if the constructor is private, such as from a factory function.

class C
{
public:
    static std::shared_ptr<C> create()
    {
        // fails
        // return std::make_shared<C>();

        return std::shared_ptr<C>(new C);
    }
private:
    C();
};
isanae
  • 3,253
  • 1
  • 22
  • 47
0

Many of the answers provide at least one unique aspect, so I decided to make a summary answer. Credit goes to @Niall, @Chris Drew, @utnapistim, @isanae, @Markus Mayr and (by proxy) Scott Meyers.

Reasons, why you might not want / might not be able to use make_shared are:

  • You have to use a custom deleter and/or even a custom allocator/new operator.
    This is probably the most common reason, especially when interfacing with 3rd party libraries. std::allocate_shared might help in some of those situations.
  • If memory is a concern and you often have weak pointers that outlive the object.
    As the managed object is created on the same chunk of memory as the control block, the memory cannot be freed until also the last weak pointer gets destroyed.
  • If you have a private constructor and e.g. only a public factory function.
    Making make_shared a friend is not a viable solution in that case, as the actual construction might happen in some helper function/class.

Regarding the code checks, the exceptions listed above are probably too many for an automatic check of a "Use make_shared whenever possible" guideline, as well as for calling a violation of that guideline a design smell in its own right.

As a bright spot on the horizon, even if a library requires custom allocator and deallocator functions and we can't / don't want to use std::allocate_shared, we can make our own version of make shared that at least encapsulates the allocation and deletion call and increases exception safety (although it most probably doesn't offer the single allocation advantage).

MikeMB
  • 20,029
  • 9
  • 57
  • 102