Should I delete pointer from `new` passed to a function which makes into a `shared

In the following code example:

#include <iostream>

class Foo{
};

class Bar{
public:
    void addFoo(Foo *foo){
        auto my_foo = std::shared_ptr<Foo>(foo);
    }
};

int main() {
    auto bar = Bar();
    bar.addFoo(new Foo());
    return 0;
}

Do I need to clean up the pointer created in main() by the bar.addFoo(new Foo) call, or will this be taken care of by Bar which creates a shared_ptr of it? My understanding is that auto my_foo = std::shared_ptr<Foo>(foo); will use the copy constructer to copy this pointer into my_foo leaving the original one dangling, is that correct?

CodePudding user response：

The very idea of a constructor taking a raw pointer is to pass the ownership to std::shared_ptr. So, no, you don't have to delete a raw pointer passed to std::shared_ptr. Doing this will lead to a double deletions, which is UB.

Note that in general passing a raw pointer is dangerous. Consider the following more generalized example:

void addFoo(Foo *foo){
        // some code which could throw an exception
        auto my_foo = std::shared_ptr<Foo>(foo);
    }

If an exception is throw before my_foo is constructed, foo will leak.

If you have no special reason to pass a raw pointer, consider the following alternative:

class Bar {
public:
    template<class... Args>
    void addFoo(Args... args){
        auto my_foo = std::make_shared<Foo>(args...);
    }  
};

int main() {
    auto bar = Bar();
    bar.addFoo();
    return 0;
}

Here you pass arguments (if you have any) to construct Foo inside addFoo() instead of constructing Foo before invoking addFoo().

Perfect forwarding of args... could be used if it is needed:

    template<class... Args>
    void addFoo(Args&&... args){
        auto my_foo = std::make_shared<Foo>(std::forward<Args>(args)...);
    }

CodePudding user response：

A raw pointer does not manage end of life, but a shared pointer does. When you create a shared pointer from a raw pointer, the shared pointer takes ownership of the object. That means that the object will be destroyed when the last shared pointer pointing to it will go out of scope.

In your code, my_foo takes ownership of the object created with new Foo(), goes out of scope when addFoo returns, and as it contains the only shared reference, correctly destroys the object.

CodePudding user response：

The code you wrote is correct. But in modern C , you should not be using raw pointers, new and delete unless you have to interoperate with code that does. If you can help it (and if question comments are any indication, you can), use smart pointers all the way through:

#include <iostream>
#include <memory>

class Foo {};

class Bar {
public:
    void addFoo(std::unique_ptr<Foo> foo) {
        auto my_foo = std::shared_ptr<Foo>(std::move(foo));
    }
};

int main() {
    auto bar = Bar();
    bar.addFoo(std::make_unique<Foo>());
    return 0;
}

Above, the addFoo member function receives the pointer as a unique_ptr, and uses std::move to transfer ownership of the pointer from the unique_ptr to the shared_ptr without copying the referent; after constructing the shared_ptr, the unique_ptr is left in an empty state. You could also have addFoo receive a shared_ptr directly, or construct the object in-place inside the member function, as in Evg’s answer.

Using unique_ptr instead of a raw pointer makes it clear that the method intends to take ownership of the allocation, and encourages callers to use smart pointers themselves, making it less likely they will forget to delete their allocations later.

CodePudding user response：

The correct, c way to do this, would be the following:

#include <iostream>

class Foo{

};

class Bar{
public:
    void addFoo(Foo foo){
        auto my_foo = std::make_shared<Foo>(foo);
    }
    
};

int main() {
    auto bar = Bar();
    bar.addFoo(Foo());
    return 0;
}

This avoids any raw pointers or naked new, and is totally exception safe. Also, std::make_shared introduces some performance benefits. One confusing thing here is that the code seems to be unnecessarily copy the Foo object, however, since C 17, due to Return Value Optimization, (RVO), you are guaranteed to have no copies at all (when passing Foo as an argument to addFoo).

CodePudding user response：

You can create the shared pointer with make_shared. If you want to construct Foo in main (e.g. because you have the paramters available there), then use make_shared at the point of construction and pass the shared_ptr on.

#include <iostream>

class Foo{
    ~Foo() { std::cout << "Foo destructed" << std::endl; }
};

class Bar{
public:
    void addFoo(std::shared_ptr<Foo> foo){
        auto my_foo = foo;
    }        
};

int main() {
    auto bar = Bar();
    bar.addFoo(std::make_shared<Foo>());
    return 0;
}

delete also calls your destructor. You can test, whether the shared pointer destructs your object or whether a delete is needed by printing out a message.