7

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 ?

Verv
  • 2,385
  • 2
  • 23
  • 37
TheWaterProgrammer
  • 7,055
  • 12
  • 70
  • 159
  • 3
    You should not mix C++ code compiled by different versions of the compiler. Doing so can lead to hard to find anomalies. – rustyx Oct 10 '17 at 17:33
  • 4
    Are you sure that you need a `shared_ptr<>` and not a `unique_ptr<>`? – simon Oct 10 '17 at 17:35
  • @RustyX as Your point would be valid if had access to changing the library's internal code. But unfortunately not – TheWaterProgrammer Oct 10 '17 at 17:37
  • @gurka I think `unique_ptr` is out of the question here? as `LibraryMethod` is returning a pointer for another guy to point to. so this implicitly bumps reference count. hence a `share_ptr` is obvious choice ? – TheWaterProgrammer Oct 10 '17 at 17:39
  • 5
    what do you mean by 'the second owner?', do you own that or not, if you own you can use either `unique_ptr` or `shared_ptr`, if you do not you cannot use either. – Slava Oct 10 '17 at 17:41
  • @Slava corrected my comment above to explain my choice of `shared_ptr` in a better way – TheWaterProgrammer Oct 10 '17 at 17:44
  • No, it does not explain better, I do not understand what 'for another guy to point to' means. – Slava Oct 10 '17 at 17:57
  • @Slava looks like you gotcha me on that. I reconsidering my choice of the smart pointer type here. please share your thoughts on this as well. – TheWaterProgrammer Oct 10 '17 at 18:06
  • 3
    Your question leaves out the most important piece of information: Who cleans up the pointer? Can the possessor simply `delete ptr;`? Do they have to call a `FreePtr` function in the library? Does the library somehow magically manage the pointer from afar? We see the library provides the pointer, but who owns it? Handy reading: https://stackoverflow.com/questions/94227/smart-pointers-or-who-owns-you-baby – user4581301 Oct 10 '17 at 18:10
  • @user4581301 you are correct. besides, if I own the raw pointer returned into a shared or a unique pointer, then exiting the app gives me a malloc error complaining `pointer being freed was not allocated`. – TheWaterProgrammer Oct 10 '17 at 18:29
  • 1
    Careful making that call. If you have not taken @CoryKramer 's answer into account, `some_class = (std::shared_ptr)LibraryMethod();` will likely blow up something along the lines of what you just reported. Do you have any library documentation you can consult to determine ownership and handling of the pointer? – user4581301 Oct 10 '17 at 18:47

4 Answers4

11

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.

Cory Kramer
  • 114,268
  • 16
  • 167
  • 218
  • I see that there is distinct difference in the way you have used braces here. I had used it around `std::shared_ptr` but you have used it around the method call that is `LibraryMethod()`. this looks a **better way** of giving the ownership of the pointer? – TheWaterProgrammer Oct 10 '17 at 17:22
  • 2
    It does not just *look* different, these two versions actually *are* different. Your method is doing a C-style cast which will likely not function correctly. This method is constructing a `std::shared_ptr` using the raw pointer as an argument. – Cory Kramer Oct 10 '17 at 17:23
  • 1
    "*constructing a shared pointer*". Is this better than doing a reset like suggested in below answer? really good suggestions here. want compare them to understand the best approach – TheWaterProgrammer Oct 10 '17 at 17:34
  • 1
    @CoryKramer In this case, it will function correctly (since the C-style cast falls back to a `static_cast` if that is possible, and it is in this case). It's of course still quite dangerous. – Angew is no longer proud of SO Oct 10 '17 at 17:38
  • 1
    If you construct it immediately following the call that's just one step. If you initialize it to `nullptr` then later populate it with `reset` that's two steps. – Cory Kramer Oct 10 '17 at 17:38
  • @CoryKramer: There is no ``reinterpret_cast(p)`` where ``p`` is a pointer and ``T`` is a class type (for details, see http://en.cppreference.com/w/cpp/language/reinterpret_cast). There are of course other dangerous possibilities, e.g. ``reinterpret_cast(p)`` will "work" if ``p`` is an lvalue. It is equivalent to ``*reinterpret_cast(&p)`` which is usually a very bad idea. – Arne Vogel Oct 12 '17 at 12:14
4

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.

elven_inside
  • 1,423
  • 1
  • 9
  • 9
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.

Angew is no longer proud of SO
  • 167,307
  • 17
  • 350
  • 455
3

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.

AMA
  • 4,114
  • 18
  • 32