0

I've been looking into smart pointers, unit testing how they manage memory and am finding and unexpected issue that all the examples recommend doing, but it creates a huge memory leak for me.

This seems to occur when I use a class that has a constructor that builds from another copy of the same class. I'll give an example.

If I have a class like:

Class foo{
public:
    //Ignore unsafe practices here
    HeavyInMemory* variable;

    foo(){
        variable = new HeavyInMemory();
    }

    foo(foo* copyThis){
        variable  = nullptr;
        if(copyThis){
            variable = new HeavyInMemory(copyThis->variable);
        }
    }

    ~foo(){
        delete variable;
    }
}

I find that I will get a huge memory leak because std::make_shared has no way to tell the difference between make_shared(args) and make_shared(new T)

Main(){
    for(int i =0; i < 100; i++{
        //Should not leak, if I follow examples of how to use make_shared
        auto test = make_shared<foo>(new foo());
    }

    //Checking memory addresses these do not match, checking total program memory use, leaks like a 
    //sieve.
}
  • Am I misunderstanding something?
  • Do the examples just not consider this as most use primitive types as examples rather than classes.
  • Does c++11 just not support the make_shared(new T) format even though I see old books like scott meyers books from 1992. It just doesn't make sense.

Also why would you use make_shared(new T) over make_shared(args)? I've seen a couple threads where people have asked this on here, but neither seemed to actually answer the question with a code example.

//As they mainly say code compiler order causes the leak but in my example this would still leak:
auto originalObject = new foo();
auto expectedDestructorWhenOutofScope = make_shared<foo>(originalObject);

//I have found if I give if the object instead it doesn't leak, but this is getting into the realms of
//hacks that may sometimes work
auto originalObject = new foo();
auto expectedDestructorWhenOutofScope = make_shared<foo>(*originalObject);

EDIT: Thanks to Igor Tandetnik I now see I am using make_shared entirely wrong. It should be used as a constructor. Thanks again Igor I appreciate it.

//Create new
auto expectedDestructorWhenOutofScope = make_shared<foo>();

//Use object already created
std::shared_ptr<Object> p2(new foo())
Natio2
  • 235
  • 1
  • 9
  • 1
    This is not how to use `make_shared`. Just construct the `std::shared_ptr`. – tkausl May 03 '20 at 11:16
  • 1
    the parameter `new foo()` passed to make_shared is never freed in the constructor called `foo(foo* copyThis)`. just use `make_shared();` – Thomas May 03 '20 at 11:19
  • 1
    `make_shared(new foo())` doesn't make sense. The arguments to `make_shared` are **not** a raw pointer to be wrapped into a `shared_ptr` - they are arguments to be passed to the constructor of the newly created object of type`foo`, and that newly created object is wrapped in `shared_ptr`. You want `auto test = std::make_shared();` which is equivalent to `auto test = std::shared_ptr(new foo());` On the other hand, `auto test = make_shared(new foo());` is equivalent to `foo* temp = new foo(); auto test = std::shared_ptr(new foo(temp)); /* leak temp */` – Igor Tandetnik May 03 '20 at 11:23
  • 1
    *"why would you use `make_shared(new T)` over `make_shared(args)`?"* It's not one vs the other. `new T` **is** `args`. It only compiles because your class `foo`, inexplicably, has a constructor that takes `foo*`. That constructor is not a copy constructor - a copy constructor would look like `foo(const foo&)`. And in fact your class does have such a copy constructor - an implicitly-defined one - that would cause double-destruction if ever used. – Igor Tandetnik May 03 '20 at 11:32
  • 1
    You ask us to follow the link and look at Exapmple 4. Well, Example 4 doesn't use `std::make_shared` at all. What is it supposed to demonstrate? – Igor Tandetnik May 03 '20 at 11:34
  • @IgorTandetnik https://en.cppreference.com/w/cpp/memory/shared_ptr/make_shared Explicitly says there is a difference between std::shared_ptr(new T(args...)) and shared_ptr make_shared( Args&&... args ) unless I am misunderstanding the notes – Natio2 May 03 '20 at 11:35
  • Valid Igor that is a misread on my part – Natio2 May 03 '20 at 11:36
  • 1
    There is a difference in that the latter may be more efficient (most of the time; less efficient in certain corner cases). There is no *observable* difference afterwardss - meaning, a program holding a resulting `shared_ptr` won't be able to tell which way it was created. – Igor Tandetnik May 03 '20 at 11:36
  • I'm more worried about the fact the memory leaks more than efficiency – Natio2 May 03 '20 at 11:41
  • 1
    The memory leak in your example is not caused by using `shared_ptr` or `make_shared` - it's caused by calling `new foo()` and then leaking the resulting pointer. Indirectly assisted by the fact that your class has an unusual constructor taking `foo*`; otherwise you'd receive a compiler error pointing out the misuse of `make_shared`. You are effectively doing `foo* test = new foo(new foo()); delete test;` The problem should be obvious when stated this way. – Igor Tandetnik May 03 '20 at 11:43
  • So to sum what what are are saying is: Giving ```make_shared``` a pointer will always make a copy and never manage the pointer it is given, so you should never see ```make_shared(new T())``` Even though I see it in many examples? OR are you saying if you make a crazy constructor like I have, that ```make_shared``` will leak on standard use? – Natio2 May 03 '20 at 11:47
  • 1
    Can you actually point to one example that uses `make_shared(new T())`? You keep saying that you see such examples often, but have yet to cite one. Anyway - I can't say you should **never** see that; I could construct an example where such a usage is safe. But this example would be very artificial, not representative of how `make_shared` is commonly used. – Igor Tandetnik May 03 '20 at 11:51
  • Ah I got you now. It's more a constructor than a holder. Sorry I was confused by seeing ```make_shared(new T())``` which had gotten me into a spin. Thank you for being patient with me Igor I appreciate the help in understanding – Natio2 May 03 '20 at 11:51

0 Answers0