Storing a raw pointer returned from a method into a smart pointer

Question

Scenario:
I am using a method from an old C++ library which returns a raw pointer to SomeClass where SomeClass is an exported class from a library header say SomeClass.h

Following is the signature of LibraryMethod that I am using:

SomeClass* LibraryMethod();

I don't have access to change the library. I am using only the binary & public header which is a typical scenario.

I dont want to use raw pointers in my part of the code. Hence, I have a shared pointer to SomeClass in my part of the code using the library API.

std::shared_ptr<SomeClass> some_class;

which I initialise like this to avoid storing a raw pointer to SomeClass

some_class = (std::shared_ptr<SomeClass>)LibraryMethod();

This basically works but I want to understand the details here

Question:
Is the above a correct technique ?
Am I causing a leak here ?
Are there any better techniques to handle such a scenario ?

Answer 1

You should actually call it as

auto some_class = std::shared_ptr<SomeClass>(LibraryMethod());

This assumes that LibraryMethod is allocating the pointer and giving ownership of the memory to you.

As currently written, you are trying to cast the raw pointer to a std::shared_ptr using a C-style cast (which can result in a reinterpret_cast). Instead you want to construct a std::shared_ptr using the returned raw pointer.

Answer 2

In your case the correct way should be to use the shared_ptr constructor:

std::shared_ptr<SomeClass> sPtr(LibraryMethod());

BUT first you should be aware what the pointer returned by LibraryMethod() really means, most libraries returns raw pointer just to say " hei, you can access this object trough this pointer, but watch out, I'm still the one in charge to manage it, so... do not delete it! "

If you are sure that after that call you are in charge to manage it, then ok, you can use a shared_ptr with the peace of mind.

Answer 3

The code works, but it's ugly and fragile. Why mix nice modern C++ (smart pointers) with something as archaic and dangerous as a C-style cast? You're much better off calling reset:

some_class.reset(LibraryMethod());

The above assumes (as your question seems to indicate) that you already have std::shared_ptr<SomeClass> some_class; declared somewhere and want to reassign it. If you're creating some_class just before the call of LibraryMethod, it's much better to directly initialise it:

std::shared_ptr<SomeClass> some_class(LibraryMethod());

This is then equivalent to CoryKramer's answer.

---

Note, however, that there's a big assumption hidden in all this code: that LibraryMethod returns a pointer to memory allocated dynamically via new and that its caller is responsible for eventually releasing that memory by calling delete.

Answer 4

Is the above a correct technique ?

Yes, given that the LibraryMethod allocates the resource with new, and the library user is responsible for deleting it. Wrapping raw pointers with smart pointers is widely-used for legacy libraries.

Am I causing a leak here ?

No, if you do it correctly. I agree with other answers, but you can also do:

std::shared_ptr<SomeClass> some_class(LibraryMethod());

Are there any better techniques to handle such a scenario ?

One thing to carefully consider is the ownership of the resource allocated by LibraryMethod. Some argue that shared_ptr is evil in the sense that it is just a nicer way of introducing a global with dependencies which are hard to track. See this talk or this one @8:40 by Sean Parent for example. You may consider using std::unique_ptr instead if there is an entity in your code that consumes and owns the resource.

Also if LibraryMethod may throw exceptions you have to handle this case accordingly.