0

This is the way i am returning smart pointer from a raw pointer.

std::shared_ptr<Geometry> Clone() const
{
    Circle *sc = new Circle(*this);
    return std::shared_ptr< Geometry >(sc);
}

Is it the correct way to return Smart pointer from raw pointer ?

suman
  • 113
  • 7

2 Answers2

4

The term "correct" is quite vague. Ok, this code should work, if you ask of that. However there are more details you should take into consideration.

First, it is better to construct the object in the same expression where you construct the smart pointer:

std::shared_ptr<Geometry> Clone() const
{
    return std::shared_ptr< Geometry >(new Circle(*this));
}

Why so? Imagine that you add one more line of code in between, and this code throws an exception... That is the benefit of smart pointers that they prevent you from memory leaks like that, and you loose this benefit in your initial version of code.

Next, I don't see the definition of the class Geometry, and it probably has a virtual destructor... But what if it doesn't? Which class would the shared_ptr destroy? The answer is: the class that shared_ptr knows. Ok, in your code there is a way for shared_ptr to get known that the actual class is class Circle (because of the shared_ptr constructor, that knows exactly the actual class of the underlying object), but it is very easy to make a mistake. One-liner solves this issue.

But there is one more (possible) improvement: std::make_shared:

std::shared_ptr<Geometry> Clone() const
{
    return std::make_shared< Circle >(*this);
}

In this case you allocate memory only once, for both the object and the counter structure. That works better unless you employ std::weak_ptr. You should be very careful using both std::make_shared and std:weak_ptr: the actual memory deallocation of the shared counter (which is the same memory that is used for the object itself in case of std::make_shared) will be done only after the last weak_ptr is destroyed. That could cause some sort of a memory leak.

So the way you use it is correct, but there is not enough information to judge what is the best idiomatic way to do that.

Dmitry Kuzminov
  • 6,180
  • 6
  • 18
  • 40
  • @t.niese, try to answer the question: what is the benefit of using `std::make_shared` comparing to direct use of `std::shared_ptr` constructor? The answer is: you may allocate memory once, for both the object and the control structure. For sure these memory locations don't overlap, but they are located in the same memory block. I've added more info regarding the usage of `weak_ptr` and `make_shared` to my answer. – Dmitry Kuzminov Jun 01 '20 at 06:41
  • @t.niese, and the problem really appears only in this case. But this problem is slightly different. If the first `shared_ptr` was created using the constructor passing the raw pointer to something already created -- you probably abuse some memory (several bytes) for the control structure. But in case of `make_shared` this is not "several bytes", but the size of the object, which could be a huge memory loss. – Dmitry Kuzminov Jun 01 '20 at 06:49
  • Ok now I get what you are referring to. I removed my comments. – t.niese Jun 01 '20 at 06:58
1

The correct way to do it is to return the following

return std::make_shared<Geometry>(new Circle(*this));

If you create the shared pointer like this

std::shared_ptr<Geometry> Clone() const
{
  Circle *sc = new Circle(*this);
  return std::shared_ptr< Geometry >(sc);
}

You end up with two allocations in memory which is not what you want.

Explanation:

A shared pointer has a wrapper with information around the pointer to keep tabs on the reference counts. When you call make_shared, those two pieces (wrapper plus the raw pointer) are guaranteed to be in the same memory block.

Otherwise it looks like this

+--+      +---------+-------------+--------------  
|sp| ---> | raw ptr | strong refs | weak refs ...
+--+      +---------+-------------+--------------  
                 \
               +---------+
               |  Circle |
               +---------+

But by using make_shared instead, it looks like this in memory

+--+      +---------+-------------+--------------  
|sp| ---> | Circle  | strong refs | weak refs ...
+--+      +---------+-------------+--------------

That is much more effective.

AndersK
  • 35,813
  • 6
  • 60
  • 86
  • As I've answered below this solution may have a drawback. – Dmitry Kuzminov Jun 01 '20 at 05:20
  • This answer is wrong as written. I suggest you remove it, because if you fix it, you would just repeat @DmitryKuzminov's answer. – j6t Jun 01 '20 at 06:15
  • @j6t wrong in which way? it is the proper way to do a shared_ptr – AndersK Jun 01 '20 at 17:35
  • It must be `return std::make_shared(*this)`. No `new` there. Perhaps you meant `return std::shared_ptr(new Circle(*this))`? But all of this has been mentioned by @DmitryKuzminov. – j6t Jun 01 '20 at 18:02