Custom std::shared_ptr<T> implementation

Question

I'd like to get a few pointers about my code.
I know that shared_ptr is up and running, and I'm reinventing the wheel but you can't say no to the school assignment.

template <typename T> class SharedPointer;

template <typename T>
void swap(SharedPointer<T> &lhs, SharedPointer<T> &rhs)
{
    lhs.swap(rhs);
}

template <typename T>
class SharedPointer {
    friend void swap<T>(SharedPointer &lhs, SharedPointer &rhs);

public:
    SharedPointer(): use_c(new std::size_t(0)), p(nullptr), deleter(Deleter()) { }
    SharedPointer(T *ptr): p(ptr), use_c(new std::size_t(1)), deleter(Deleter()) { }
    SharedPointer(T *ptr, const std::function<void(*)> &del):
            p(ptr), use_c(new std::size_t(1)), deleter(del) { }
    SharedPointer(const SharedPointer &rhs): 
            use_c(rhs.use_c), p(rhs.p), deleter(rhs.deleter) { ++*use_c; }

    SharedPointer(SharedPointer &&) noexcept;
    SharedPointer& operator=(const SharedPointer &);
    SharedPointer& operator=(SharedPointer &&) noexcept;
    ~SharedPointer() { free(); }

    std::size_t use_count() { return *use_c; }

    bool unique() const { return *use_c == 1; }

    operator bool() const { return p != nullptr; }

    void reset() noexcept { free(); use_c = new std::size_t(0); };
    void reset(T*);
    void reset(T*, const std::function<void(T*)>&);

    void swap(SharedPointer<T>&);

    T* get() const { return p; }

    T& operator*() const { return *p; }
    T* operator->() const { return &*p; }

private:
    std::size_t *use_c;
    T *p;
    std::function<void(T*)> deleter;

    void free();
};

template <typename T>
SharedPointer<T>::SharedPointer(SharedPointer &&rhs) noexcept:
        use_c(rhs.use_c), p(rhs.p), deleter(std::move(rhs.deleter))
{
    rhs.use_c = new std::size_t(0);
    rhs.p = nullptr;
}

template <typename T>
SharedPointer<T> &SharedPointer<T>::operator=(const SharedPointer &rhs)
{
    ++*rhs.use_c;
    free();
    this->use_c = rhs.use_c;
    this->p = rhs.p;
    this->deleter = rhs.deleter;
    return *this;
}

template <typename T>
SharedPointer<T> &SharedPointer<T>::operator=(SharedPointer &&rhs) noexcept
{
    swap(rhs);
    rhs.free();
    rhs.use_c = new std::size_t(0);
    return *this;
}

template <typename T>
void SharedPointer<T>::reset(T *rhs)
{
    if (p != rhs) {
        free();
        p = rhs;
        use_c = new std::size_t(1);
    }
}

template <typename T>
void SharedPointer<T>::reset(T *rhs, const std::function<void(T *)> &del)
{
    if (p != rhs) {
        reset(rhs);
        deleter = del;
    }
}

template <typename T>
void SharedPointer<T>::swap(SharedPointer<T> &rhs)
{
    using std::swap;
    swap(this->use_c, rhs.use_c);
    swap(this->p, rhs.p);
    swap(this->deleter, rhs.deleter);
}

template <typename T>
void SharedPointer<T>::free()
{
    if (p && --*use_c == 0) {
        deleter(p);
        delete use_c;
    } else if (!p) {
        delete use_c;
    }
    use_c = nullptr;
    p = nullptr;
}

I've yet to add here some non-member operators, but I guess that's trivial and it's no use to litter this post so much.
I've tried to test it here and there and it looks like it works but I'm unsure about copy-and-swap and few other things.
It uses this functor class as the default deleter, if anyone is wondering what's that about.

class Deleter {
public:
    template <typename T> void operator()(T *p) const;
};

template <typename T> void Deleter::operator()(T *p) const
{
    delete p;
}

So basically I'd like to ask your help with the copy-and-swap idiom - if this implementation is correct and if it's needed at all and the overall coding style.

Answer 1

Nice first post!

There are things that std::unique_ptr provides which are absent here, notably the aliasing constructor (so you can own the containing object of what's referenced) and weak pointers (so cached values don't cause unwanted object retention) but otherwise, this is pretty good.

Omissions

I had a couple of compilation errors, because it's missing #include <functional>. Also, the deleter ought to have a specialization for array types (which need delete[] p; instead of delete p;).

Naming

On naming, I'd probably choose something like release() for what you've called free() - but that's probably just because of half a lifetime writing C.

Copy and swap

You mention the copy-and-swap idiom; this is how you'd use it for your assignment operator:

template<typename T>
SharedPointer<T>& SharedPointer<T>::operator=(SharedPointer rhs)
{
    swap(rhs);
    return *this;
}

Notice that we pass rhs by value, which means we're getting the copy constructor to do all the work for us. Having swapped, rhs has our old value, which is then destructed at the end of the function, so we don't have to duplicate the work of the constructor.

Move-assignment

Move assignment doesn't need to reset rhs - just perform the swap, and expect the moved-from value to be destructed soon:

template <typename T>
SharedPointer<T> &SharedPointer<T>::operator=(SharedPointer &&rhs) noexcept
{
    swap(rhs);
    return *this;
}

Swap

The swap function doesn't need to be a friend, as it uses only the public interface of the shared pointer.

Use default arguments to simplify

You can reduce the number of constructors, like this:

SharedPointer(T *ptr = nullptr,
              const std::function<void(*)> &del = Deleter())
    : p(ptr),
      use_c(new std::size_t(ptr!=nullptr)),
      deleter(del)
{ }

This is also true for the reset() method.

Reset should change deleter

The condition in reset() skips changing the deleter if there's no change to the pointer. But we might be calling reset() for exactly that purpose. Also, if a null pointer is passed as the new value, it's treated the same as a non-null pointer. Both of these problems can be avoided by using swap() and default arguments (to re-use our constructor and destructor):

    // declaration
    void reset(T* = nullptr, const std::function<void(T*)>& = Deleter());

template <typename T>
void SharedPointer<T>::reset(T *rhs, const std::function<void(T*)>& del)
{
    swap(SharedPointer(rhs, del));
}

Answer 2

Let's first take a look at your data-structures.

1. Your shared control-block consists only of a strong reference-count, so you have no support for WeakPointers, which are somewhat important for manually breaking cycles.

2. Your deleter is stored as a std::function<void(T*)> directly in the SharedPointer itself, meaning that:

   • Copying/moving a SharedPointer gets more expensive, potentially involving allocating dynamic memory for the std::function.
   • You have to have the correct pointer on hand for passing to the std::function to free the managed object. So, no pointing to a sub-object, dependent object, or base class subobject. That's fairly restrictive.

3. Nothing much to be said about the pointer to the pointee itself.

How should it be organized instead?

class SharedPointerControlblock {
    virtual void destroy() noexcept = 0; // destroys pointee
    std::size_t cWeak = 1; // weak reference count. Destroy cb if 0
    std::size_t cStrong = 1; // strong reference count. Destroy pointee if 0
protected:
    virtual ~SharedPointerControlblock() = default;
public:
    // fastAddRef() tryAddRef() release() weakAddRef() weakRelease()
};

Create final derived classes as needed.

4. There's no reason to define swap(SP&, SP&) before the class is defined. Doing so just leads to more typing and worse reading.

5. swap should be noexcept.

6. There's no reason to make a function just using the public interface a friend. Really.

7. A default-constructed SP should not allocate any memory at all.

8. The deleter used should be a template-argument of the ctor used instead of provided via std::function for efficiency and versatility. Actually, better make it an allocator than a deleter, and use the added possibilities.

9. The best you can do with your move-ctor is removing every trace of its existence, and rely on the copy-ctor. That has a lower chance of throwing an exception and won't abort the program if it does. The only problem with the copy-ctor is that it must copy a std::function and is thus not noexcept.

10. Remember that a moved-from object can have any arbitrary valid state. Don't waste any effort to guarantee more.

constexpr SharedPointer() noexcept : use_c(), p() {}
SharedPointer(SharedPointer&& o) noexcept : SharedPointer() { swap(o); }
SharedPointer(const SharedPointer& o) use_c(o.use_c), p(o.p), deleter(o.deleter) { if(use_c( ++*use_c; }
SharedPointer& operator=(SharedPointer&& o) noexcept { swap(o); return *this; }
SharedPointer& operator=(SharedPointer o) { swap(o); return *this; }

11. &*p Hm, what? Why not simply p as it's a raw pointer already.

12. Your reset() should not concern itself with whether the new pointer is the old pointer. If it is, and not nullptr, that's programmer error.

13. reset() should not ever leave the object in a halfway state, ever.

14. You should only ever delete the use-count if it's 0, and not make its decrementing dependent on p.

Answer 3

Things that are broken

Must always take ownership

When you create a smart pointer it must always take ownership if things go wrong. If the constructor fails in some way then you must delete the passed object (becuase the destructor is not run if the constructor does not complete).

The problem with this is:

SharedPointer(T *ptr)
    : p(ptr)
    , use_c(new std::size_t(1))
    , deleter(Deleter())
{ }

You call new. Unfortunately this can fail with an exception. If this happens the pointer is leaked. Once the user has called your constructor they expect you to have ownership. So you must delete that pointer if new throws an exception.

SharedPointer(T *ptr)
try {
    : p(ptr)
    , use_c(new std::size_t(1))
    , deleter(Deleter())
{ }
} catch(...) {
    delete ptr;
    throw;
}

Accidentally creating a shared pointer without knowing it.

If you have a function/method that has a shared pointer as a parameter. Then the compiler will auto convert a pointer into shared pointer before the call. Thus taking ownership of the pointer. When the function exits the shared pointer is destroyed and your pointer deleted. If you did not realize that the function was taking a shared pointer then your pointer is destroyed.

void myCode(SharedPointer<int> data) {
    // Do stuff with int
}

int main()
{
    int* x = new int(5);
    myCode(x);  // will delete the pointer x
                // did you expect that?
                // The compiler will not complain.
    *x += 2; 
}

Assignment operator not exception safe

template <typename T>
SharedPointer<T> &SharedPointer<T>::operator=(const SharedPointer &rhs)
{
    ++*rhs.use_c;

    // What happens if free() throws an exception?
    free();
    // Well the current object is fine (it will be correctly destroyed).
    // But the object represented by `rhs` has a incorrect count that will
    // will never reach zero.


    this->use_c = rhs.use_c;
    this->p = rhs.p;
    this->deleter = rhs.deleter;
    return *this;
}

The assignment operator should work in three distinct phases.

1) Allocate resources into temp objects  (dangerous might throw)
2) Do an exception safe swap of the temp object and the current objets state.
3) Release resources (now in temp object). Again dangerous but the object
   is in a consistent state.

The classic implementation of these three steps is the Copy and Swap Idiom.

 {
     Shared<object>  tmp(rhs);   // make a copy into a temp object.
     swap(tmp, *this);           // swap the temp object and this.
 }
 // Let the tmp object go out of scope and destroy its content.
 // This was the original content of this.

Things that are missing

Initializing with nullptr

You can't initialize your smart pointer with nullptr. It may not seem necessary but a lot of the template code initializes with a zero object.It would not surprise me that there is a nullptr initialization for pointer types in there somewhere.

SharedPointer<int>    x(nullptr);   // Fails to compile
SharedPointer<int>    y(reinterpret_cast<int*>(nullptr)); // Seems a bit long winded.

Derived Types

One of the big features in C++ is derived types.

Animal* a = new Cat;

Being able to assigning Cat(s) to Animals(s) is normal operation.

Cat*    c = new Cat;
Animal* a = c;        // Should work the same.

Or more likely

void makeNoise(Animal* a) { a->makeNoise();}

// Later.
Cat*    c = new Cat;
makeNoise(c);

If we translate this directly into your shared pointer it will no longer compiler.

SharedPointer<Animal> a(new Cat);   // OK this works

Being able to assigning Cat(s) to Animals(s) is normal operation.

SharedPointer<Cat>    c(new Cat);
SharedPointer<Animal> a(c);        // Should work the same.
                                   // But this is a compiler error.

Or more likely

void makeNoise(SharedPointer<Animal> a) { a->makeNoise();}

// Later.
SharedPointer<Cat>    c(new Cat);
makeNoise(c);                           // Now fails to compile.

Things that look strange

Testing if a pointer is already stored.

template <typename T>
void SharedPointer<T>::reset(T *rhs)
{
    if (p != rhs) {
        free();
        p = rhs;
        use_c = new std::size_t(1);
    }
}

When you give a pointer to a shared pointer you are asking it to take ownership. If it has ownership you should not have any copies of the pointer running around your code (otherwise what is the point of the shared pointer).

So it seems that this is a logical error in your code. If somebody assigns a pointer to a shared pointer that happens to be the same as the already contained pointer there is something seriously wrong with the users code. I would prefer this test did not exist or if it does exist it throws an exception to indicate the enormity of the problem.

The second reset

template <typename T>
void SharedPointer<T>::reset(T *rhs, const std::function<void(T *)> &del)
{
    if (p != rhs) {
        reset(rhs);
        deleter = del;
    }
}

Since this is a call to the previous reset() that already tests for assignment of self there is no need to call here. Simply call reset(rhs) and the child version will test for self assignment.

Prefer not to use this->

template <typename T>
void SharedPointer<T>::swap(SharedPointer<T> &rhs)
{
    using std::swap;
    swap(this->use_c, rhs.use_c);
    swap(this->p, rhs.p);
    swap(this->deleter, rhs.deleter);
}

There is no need for this-> in this context.

Unlike Java it is very unusual to use this-> in C++ code. Using it also leads to errors that compilers can not detect. The only reason to use this->is to distinguish a local variable from a member variable that has the same name. This is called shadowing. The problem is that the compiler can not tell if you meant the local variable or the member so there is no way to automatically detect when you accidentally forget to use this->.

On the other hand the compiler can easily detect shadowed variables and generate a warning. A compiler warning is a logical error in your code that you should fix (your code compiles with zero warnings when you add -Wall -Wextra?).

Anyway, not using shadowed variables makes your code easier to read and understand and you will never accidentally assign to the wrong variable (as they have completely different names).