Implementing my own shared-ownership double-indirecting smart-pointer

Question

I am implementing my own double-indirecting shared-ownership smart-pointer because I needed a way to replace the managed object a group of smart-pointers is pointing to with a new object (it's the method reset_all in the code below).

I tried to keep this as close as I could to the std::shared_ptr's interface, while avoiding things I don't understand / need like aliasing and conversion from other smart-pointer types.

//(it's all in an header file)
#pragma once
template <typename T>
class Manager {
 template <typename U>
 friend class SharedPtr;
 T* managed;
 unsigned int counter;
 Manager(T* new_ptr):
 managed(new_ptr), counter(1) {}
 ~Manager() = default;
 Manager<T>* own() {
 ++counter;
 return this;
 }
 void disown() {
 if (!(--counter)) {
 delete managed;
 delete this;
 }
 }
};
template <typename T>
class SharedPtr {
 public:
 SharedPtr() = default;
 SharedPtr(T* new_ptr) {
 if (new_ptr) {
 manager = new Manager<T>(new_ptr);
 }
 }
 SharedPtr(const SharedPtr &rhs):
 manager(rhs.manager->own()) {}
 SharedPtr(SharedPtr &&rhs):
 manager(rhs.manager) {
 rhs.manager = nullptr;
 }
 SharedPtr& operator=(const SharedPtr &rhs) {
 if (manager) {
 manager->disown();
 }
 manager = rhs.manager->own();
 return *this;
 }
 SharedPtr& operator=(SharedPtr &&rhs) {
 if (manager) {
 manager->disown();
 }
 manager = rhs.manager;
 rhs.manager = nullptr;
 return *this;
 }
 void swap(SharedPtr &rhs) {
 Manager<T>* tmp = manager;
 manager = rhs.manager;
 rhs.manager = tmp;
 }
 void reset() {
 if (manager) {
 manager->disown();
 }
 manager = nullptr;
 }
 void reset(T* new_ptr) {
 if (manager) {
 manager->disown();
 }
 manager = new Manager<T>(new_ptr);
 }
 T* get() {
 if (manager) {
 return manager->managed;
 }
 return nullptr;
 }
 T& operator*() {
 return *(manager->managed);
 }
 T* operator->() {
 return manager->managed;
 }
 unsigned int use_count() {
 if (manager) {
 return manager->counter;
 }
 return 0;
 }
 bool unique() {
 return use_count() == 1;
 }
 operator bool() {
 return (bool) manager;
 }
 void reset_all(T* new_ptr) {
 if (manager) {
 delete manager->managed;
 manager->managed = new_ptr;
 return;
 }
 manager = new Manager<T>(new_ptr);
 }
 ~SharedPtr() {
 if (manager) {
 manager->disown();
 }
 }
 private:
 Manager<T>* manager = nullptr;
};

Answer 1

Design Review

You have decided that the managed object can potentially be null (when the object being managed is null.

Personally I would (consider) always having a managed object. That will make the rest of your code simpler to write (I like simpler code) as there will never be a nullobject

Of course the other side of the argument is that allowing a null managed object allows better resource management. I have not done the mathsso I have not come to a conclusion but that may be worth doing and putting into a comment in the code.

Code Review

Namespace

You should put all your code in its own namespace. This also helps in making your include guards unique.

Prefer include guards

This pragma is not supported by all compilers.

#pragma once

So prefer to use a system that works without change everywhere.

Manager

The class Manager is not used outside of class SharedPtr so personally I would make it a private member class of this SharedPtr.

Though you don't do any copying it is obvious that the Manager is non copyable. So it would be a good idea to make it explicitly non copyable to make sure you don't do so accidently.

Doing delete this; is very dangerous.

 delete this;

I would rather change the interface so that the caller of disown() does the actuall delete of the manager object.

bool disown() {
 return ((--counter) == 0);
}
~Manager() {
 delete managed;
}

Single Argument Constructor

When you have single argument constructors you have to be very careful. The compiler will eagerly convert one type to another using a single argument constructor (and this can be an issue when you least expect it).

Examine this:

actionOne(SharedPtr<int> const& val){/*Do Stuff*/}
int main()
{
 int b;
 actionOne(&b); // Caboom.
}

Here the compiler sees that it has int* as a parameter. But the only version of the function it has requires SharedPtr<int> const but that is easily achieved by constructing the SharedPtr<int> directly in place and it will do so. In this case this is very dangerous as the pointer was not dynamically allocated but is going to be deleted.

So make your single argument constructor explicit (especially if they are taking ownership).

 explicit SharedPtr(T* new_ptr);

Explicit construction with nullptr

With the introduction of nullptr we have an object that can be used for the null value. But I see no constructor that covers an explicit construction with a nullptr.

 explicit SharedPtr(nullptr_t);

Move Semantics.

It is a good idea to mark your move constructor and move assignment operator as noexcept. This is because (as they are not creating resources they usually are) but also this allows some good optimizations with standard library containers.

 SharedPtr(SharedPtr &&rhs) noexcept;
 SharedPtr& operator=(SharedPtr &&rhs) noexcept;
 void swap(SharedPtr &rhs) noexcept;

Why re-write std::swap?

 void swap(SharedPtr &rhs) {
 Manager<T>* tmp = manager;
 manager = rhs.manager;
 rhs.manager = tmp;
 }
 void swap(SharedPtr &rhs) noexcept {std::swap(manager, rhs.manager);}

What about the null object?

 void reset(T* new_ptr) {
 if (manager) {
 manager->disown();
 }
 manager = new Manager<T>(new_ptr);
 }

In most situations a nullptr results in a null manager object. But this reset creates a manager object for the null pointer. I would try and be consistent.

Const correctness

If a function does not mutate the state of the object. Then it should be marked as const.

 unsigned int use_count() const;
 bool unique() const;

Explicit bool cast

The current cast to bool is a bit dangerous as it the compiler will use it when trying to get types to match for some operations.

Consider:

SharedPtr<int> val1(new int(4));
SharedPtr<int> val2(new int(8));
if (val1 == val2) {
 std::cout << "Val1 and Val2 match\n"; // Will print out.
}

This is because the compiler sees that it can do the comparison by converting the two values to bool and then doing the comparison.

You can fix this by marking the function explicit.

 explicit operator bool() const;

This will only allow conversion to bool when an object is used explicitly in a boolean context or explicitly cast to a bool.

if (val1) // This works as expected as the expression for an if
 // statement requires a boolean and thus it is considered
 // a boolean context and will allow the bool cast operator
 // to fire even if it is explicit.
{
 std::cout << "Val1 is null\n";
}

Dont Like this.

You are mixing business and resource logic.

 void reset_all(T* new_ptr) {
 if (manager) {
 delete manager->managed;
 manager->managed = new_ptr;
 return;
 }
 manager = new Manager<T>(new_ptr);
 }

If the user wanted to set the value being managed to another value you can just use the managed objects assignment operator.

 SharedPtr<int> x(new int(6));
 x.reset_all(new int(5));
 // or
 *x = 5;

Plug for me.

I wrote up a detailed description of all the issues with writing a smart pointer here:

unique ptr
shared ptr
constructor

• user6245072: Now that I think about it, with the current implementation, if I create an empty pointer and then another pointer out of the first one, if I call reset_all on the first one the other will be unaffected. So yeah, I'm switching to always having a manager. About the "Don't like reset_all" point, using *ptr = 5 is always fine for builtin types, but it gets worse with user-defined types. And what if you're manipulating pointers to a polyphormic abstract class, and want to change the type of the managed object between two different derived types? This is the easiest way to do that.
• user6245072: A question. What are the benefits of having a specific constructor for nullptr_t? Isn't that covered by explicit SharedPtr(T* new_ptr)?
• Loki Astari: @user6245072: Read this: lokiastari.com/blog/2015/01/23/…

Answer 2

1. I wouldn't call it SharedPtr because the contrast to a std::shared_ptr is too big, and the name too similar.
   Maybe DoublePtr or alike to emphasize the double-indirection which is the crucial feature...
2. I would make Manager a private non-template member of your class, because it's an implementation-detail. Also, I would make it a dumb struct and put the useful function it contains (disown) into the private part of DoublePtr.
3. While having a default-ctor without allocation might be enough of a boon to allow a resourceless state (it probably is), there's no reason to disallow creating a pointer-group which starts as nullptr.
   Thus I would remove the check in the ctor.
4. op= is wrong, try self-assignment. It should be disown after own.
5. op=(&&) would be better as a simple swap.
6. op(bool) is wrong: Use get() != nullptr.
7. reset and reset_all could easily be upgraded from basic to strong exception-safety (commit-or-rollback), just create a new DoublePtr and swap.
8. Many of your functions can and should be marked noexcept.
9. Also, pure observing methods should be marked const.

Answer 3

In operator bool() I'd prefer an explicit not-null check like:

 operator bool() {
 return manager != nullptr;
 }

Rather than casting the pointer. The rest looks fine to me.