Should I initialize a shared_ptr inside or outside the class constructor?

Question

Given the following example code:

#include <memory>

class Foo {
public:
    Foo(std::shared_ptr<int> p);
private:
    std::shared_ptr<int> ptr;
};

Foo::Foo(std::shared_ptr<int> p) : ptr(std::move(p)) {
}

class Bar {
public:
    Bar(int &p);
private:
    std::shared_ptr<int> ptr;
};

Bar::Bar(int &p) : ptr(std::make_shared<int>(p)) {
}

int main() {
    Foo foo(std::make_shared<int>(int(256)));
    Bar bar(*new int(512));

    return 0;
}

Both Foo and Bar work correctly. However, are there any differences between creating the shared_ptr when calling the constructor and then transferring ownership with std::move and just passing a reference to the object and delegating the creation of the shared_ptr to the class constructor?

I assume the second way is better since I don't have to move the pointer. However, I've mostly seen the first way being used in the code I'm reading.

Which one should I use and why?

Answer 1

Both can work correctly, but the way you use them in main is not consistent.

When I see a constructor taking a reference (like Bar(int& p)) I expect Bar to hold a reference. When I see Bar(const int& p) I expect it to hold a copy. When I see a rvalue ref (not universal, like Bar(int&& p) I expect p not "surviving its content" after passed. (well... for an it that's not that meaningful...).

In any case p holds an int, not a pointer, and what make_shared expects are the parameter to forwarded to the int constructor (that is ... an int, not int*).

Your main so have to be

Foo foo(std::make_shared<int>(int(256)));
Bar bar(512);

This will make bar to hold a dynamically allocated sharable copy of the value 512.

If you do Bar bar(*new int(512)) you make bar to hold a copy of your "new int", whose pointer gets discarded, and hence the int itself leaked.

In general, expressions like *new something should sound as "no no no no no ..."

But you Bar constructor has a problem: to be able to take a constant or an expression returned value, it must take a const int&, not an int&

Answer 2

If your intention is to take sole ownership of a heap-allocated object I suggest you accept a std::unique_ptr. It documents the intention clearly and you can create a std::shared_ptr from the std::unique_ptr internally if you need to:

#include <memory>

class Foo {
public:
    Foo(std::unique_ptr<int> p);
private:
    std::shared_ptr<int> ptr;
};

Foo::Foo(std::unique_ptr<int> p) : ptr(std::move(p)) {
}

int main() {
    Foo foo(std::make_unique<int>(512));
}

Don't do Bar, it is error prone and fails to describe your intent (if I understand your intent correctly).

Answer 3

The second way is incorrect; it leaks memory. With

Bar::Bar(int &p) : ptr(std::make_shared<int>(p)) {
}

....

Bar bar(*new int(512));

the shared_ptr made by std::make_shared takes the value of p (which is 512) to build a new shared pointer that's responsible for a new piece of memory; it does not accept responsibility for the memory address where p lies. This piece -- the one you allocated in main -- is then lost. This particular piece of code would work with

                        // +---------------------- building a shared_ptr directly
                        // v                v----- from p's address
Bar::Bar(int &p) : ptr(std::shared_ptr<int>(&p)) {

...but look at that. That is pure evil. Nobody expects that a reference parameter to a constructor requires that it references a heap object of which the new object will take responsibility.

You could more sanely write

Bar::Bar(int *p) : ptr(p) {
}

....

Bar bar(new int(512));

In fact you could give Foo a second constructor that does that, if you wanted. It's a bit of an argument how clear the function signature makes it that the pointer has to be to a heap-allocated object, but std::shared_ptr offers the same, so there's precedent. It depends on what your class does whether this is a good idea.

Answer 4

Assuming that you used int just as an example, and there is a real resource instead, then it depends on what you want to achieve.

The first is a typical dependency injection, where the object is injected through the constructor. It's good side is that it can simplify unit testing.

The second case just creates the object in the constructor, and initializes it using values passed through the constructor.

---

By the way, take care how you initialize. This :

Bar bar(*new int(512));

is causing a memory leak. A memory is allocated, but never deallocated.

Answer 5

Foo is correct.

Bar is an abomination. It involves a memory leak, unsafe exception behaviour and an un-necessary copy.

EDIT: explanation of memory leak.

Deconstructing this line:

Bar bar(*new int(512));

results in these operations:

1. call new int(512) which results in a call to operator new, returning a pointer to an int on the heap (memory allocation).
2. dereference the pointer in order to provide a const reference for the constructor of Bar
3. Bar then constructs its shared_ptr with one returned by make_shared (this part is efficient). This shared_ptr's int is initialised with a copy of the int passed by reference.
4. then the function returns, but since no variable has recorded the pointer returned from new, nothing can free the memory. Every new must be mirrored by a delete in order to destroy the object and deallocate its memory.
5. therefore memory leak

Answer 6

It depends on what you want to achieve.

If you need a shared_ptr internally, because you want to share the object with some other objects your create later, the second way may be better (except for that horrible constructor call obviously).

If you want shared ownership of an existing object (this is the more common case, really), you don't have a choice and you need to use the first way.

If neither of these applies, you probably don't need a shared_ptr in the first place.