C++ Shared_Ptr implementation

Question

I reinvented a c++ smart pointer, shared_ptr to be precise. It is meant for practice purpose and does not attempt to replace the standard implementation. To the best of my knowledge, the code works as expected. I decided to skip the custom deleter because I want to keep things simple for now. I would love feedbacks and constructive criticism.

Here is the implementation

shared.hpp

#ifndef SHARED_PTR_H_
#define SHARED_PTR_H_

#include <cstddef>

namespace smp
{
    template<typename T>
    class shared_ptr 
    {
        public:
            explicit shared_ptr(T* ptr = nullptr);
            ~shared_ptr();
            shared_ptr(shared_ptr& ptr);
            shared_ptr operator=(shared_ptr& ptr);
            shared_ptr(shared_ptr&& ptr) noexcept;
            shared_ptr operator=(shared_ptr&& ptr) noexcept;

            T operator*();
            T* operator->() const;

            size_t use_count() const;
            void reset();
            T* get();
            bool unique() const;
            operator bool() const;
            
        private:
            T* obj_ptr;
            struct RefCount {
                size_t use_count_;
                T* object_;
            }* ref_cnt;

    };

    template<typename T>
    shared_ptr<T> make_shared(T object);
}

#endif /* SHARED_PTR_H_ */

shared.cpp

#include "shared.hpp"

#include <algorithm>
#include <iostream>

namespace smp
{
    template<typename T>
    shared_ptr<T>::shared_ptr(T* ptr)
        : obj_ptr{ptr}, ref_cnt{new RefCount()}
    {
        ref_cnt->object_ = ptr;
        if(ptr)
            ++ref_cnt->use_count_;
    }

    template<typename T>
    shared_ptr<T>::shared_ptr(shared_ptr& ptr)
        : ref_cnt{new RefCount()}  
    {
        obj_ptr = ptr.obj_ptr;
        ref_cnt = ptr.ref_cnt;

        ++ref_cnt->use_count_;
    }

    template<typename T>
    shared_ptr<T> shared_ptr<T>::operator=(shared_ptr& ptr)
    {
        obj_ptr = ptr.obj_ptr;
        ref_cnt = ptr.ref_cnt;
        ++ref_cnt->use_count_;

        return *this;
    }

    template<typename T>
    shared_ptr<T>::shared_ptr(shared_ptr&& ptr) noexcept
        : obj_ptr{nullptr}, ref_cnt{new RefCount()}
    {
        obj_ptr = std::move(ptr.obj_ptr);
        ref_cnt = std::move(ptr.ref_cnt);

        ptr.reset();
    }

    template<typename T>
    shared_ptr<T> shared_ptr<T>::operator=(shared_ptr&& ptr) noexcept
    {
        if(obj_ptr)
            reset();
        obj_ptr = std::move(ptr.obj_ptr);
        ref_cnt = std::move(ptr.ref_cnt);

        ptr.obj_ptr = nullptr;

        return *this;
    }

    template<typename T>
    shared_ptr<T> make_shared(T object)
    {
        return shared_ptr<T>(new T{object});
    }

    template<typename T>
    T shared_ptr<T>::operator*()
    {
        return *obj_ptr;
    }

    template<typename T>
    T* shared_ptr<T>::operator->() const
    {
        return &*obj_ptr;
    }

    template<typename T>
    size_t shared_ptr<T>::use_count() const
    {
        return ref_cnt->use_count_;
    }

    template<typename T>
    void shared_ptr<T>::reset()
    {
        obj_ptr = nullptr;

        if(ref_cnt)
            --ref_cnt->use_count_;

        if(ref_cnt->use_count_ == 0)
            ref_cnt->object_->~T();
    }

    template<typename T>
    T* shared_ptr<T>::get()
    {
        return obj_ptr;
    }

    template<typename T>
    bool shared_ptr<T>::unique() const
    {
        return (ref_cnt->use_count_ == 1);
    }

    template<typename T>
    shared_ptr<T>::operator bool() const 
    {
        return (obj_ptr != nullptr);
    }
    
    template<typename T>
    shared_ptr<T>::~shared_ptr()
    {
        if(obj_ptr)
            reset();
    }
}

Answer 1

Overview

Templated classes "normally" should provide all the code at the point of usage. As a result you should probably not have a separate shared.cpp. Though some people put the definitions in a separate shared.tpp file that is included by the header file.

Code Review:

You are missing a constructor for nullptr.

Though nullptr can be converted to any other type the compiler can not know what type you want in this situation so you would need to be explicit. But it would be nice to allow auto conversion from nullptr to the correct shared pointer type.

 void myFunc(smp::shared_ptr<int>&& ptr) { /* Stuff */}

 int main()
 {
     myFunc(nullptr); // This fails.
                      // as you defined an explicit constructor.
                      // But this is totaly safe so it would be nice if it
                      // simply worked rather than forcing the long hand.
 }

So I would add a constructor for this situation.

 smp::shared_ptr::shared_ptr(std::nullptr_t);

---

Non standard copy constructor and move constructor:

            shared_ptr(shared_ptr& ptr);           // Would expect const ref
            shared_ptr operator=(shared_ptr& ptr); // 

In the current state these can not catch temporary values.

---

Most people forget the noexcept. So well done.

            shared_ptr(shared_ptr&& ptr) noexcept;
            shared_ptr operator=(shared_ptr&& ptr) noexcept;

---

You should probably return a reference here:

            T operator*();

Otherwise you are going to force a copy of the internal object as it is returned. Just like the operator-> this does not affect the state of the shared pointer so this is const.

In std::shared_ptr this is a noexcept operation. I could personally argue over that. But smarter people than me wrote the standard so I would go with them.

            T&       operator*() const noexcept;

---

Good. T* operator->() const;

But like the operator* can be noexcept.

            T* operator->() const noexcept;

---

You don't want this accidentally converting in non boolean contexts.

            operator bool() const;

In this situation:

      smp::shared_ptr<int>   x(new int{6});
      smp::shared_ptr<float> y(new float{5});

      if (x == y) {
          // Because of your bool operator
          // they are converted to bool (for both this is true)
          // resulting in this code saying they are equal.
          std::cout << "Equal\n";
      }

Use the explicit here:

      explicit operator bool() const;

---

The issue I have hear is that if the new fails (for RefCount) then you leak the pointer.

Once you had the pointer to the constructor of a shared pointer you are seeding all responsibility of the pointer. That mean if the object fails to correctly construct you need to make sure the passed pointer is deleted. So in the situation of new failing you need to call delete on the pointer.

template<typename T>
shared_ptr<T>::shared_ptr(T* ptr)
    : obj_ptr{ptr}, ref_cnt{new RefCount()}
{
    ref_cnt->object_ = ptr;
    if(ptr)
        ++ref_cnt->use_count_;
}

Add a try catch block to the initializer list:

template<typename T>
shared_ptr<T>::shared_ptr(T* ptr)
try
    : obj_ptr{ptr}
    , ref_cnt{new RefCount()}
{
    ref_cnt->object_ = ptr;
    if(ptr)
        ++ref_cnt->use_count_;
}
catch(...)
{
    delete ptr;
}

---

If you are copying a shared_ptr then you should be using the same ref_cnt not a brand new one.

You should prefer to do as much work in the initializer list as possible to prevent multiple initializations.

    shared_ptr<T>::shared_ptr(shared_ptr& ptr)
        : ref_cnt{new RefCount()}  
    {
        obj_ptr = ptr.obj_ptr;
        ref_cnt = ptr.ref_cnt;

        ++ref_cnt->use_count_;
    }

// Fixed:

    shared_ptr<T>::shared_ptr(shared_ptr& ptr)
        : obj_ptr(ptr.obj_ptr)
        , ref_cnt{ptr.ref_cnt}  
    {
        if (obj_ptr) {
            ++ref_cnt->use_count_;
        }
    }

---

Here you forget to decrement the ref_counter of the previous object (so potentially allowing a leak). I would use the copy and swap idiom to provide the strong exception guarantee (it also covers self assignment).

    template<typename T>
    shared_ptr<T> shared_ptr<T>::operator=(shared_ptr& ptr)
    {
        obj_ptr = ptr.obj_ptr;  // Old object potentially leaked.
        ref_cnt = ptr.ref_cnt;  // Old ref count potentially leaked.
        ++ref_cnt->use_count_;

        return *this;
    }

---

Again incorrectly increment the counter.

    template<typename T>
    shared_ptr<T>::shared_ptr(shared_ptr&& ptr) noexcept
        : obj_ptr{nullptr}, ref_cnt{new RefCount()}
    {
        obj_ptr = std::move(ptr.obj_ptr);
        ref_cnt = std::move(ptr.ref_cnt);

        ptr.reset();
    }

The reset will decrement the counter. Since you have moved the object there has been no decrement.

Simpler to simply set this one up as null and then swap.

---

This can fail.

    template<typename T>
    shared_ptr<T> shared_ptr<T>::operator=(shared_ptr&& ptr) noexcept
    {
        if(obj_ptr)
            reset();

If ptr is the only copy. Then calling reset is going to free the object. Thus any other code is going to now manipulate a freeed object.

        obj_ptr = std::move(ptr.obj_ptr);
        ref_cnt = std::move(ptr.ref_cnt);


        ptr.obj_ptr = nullptr;
        // If you don't reset the ref count object then
        // eventually `ptr` is going to go out of scope and
        // its destructor is going to mess with the ref count object.

Simpler to simply set this one up as null and then swap.

---

Why does the shared object have to be copied constructed?

    template<typename T>
    shared_ptr<T> make_shared(T object)
    {
        return shared_ptr<T>(new T{object});
    }

Would be nive to allow the type T to be constructed using any of its normal constructors that take parameters.

    template<typename T, typename... Args>
    shared_ptr<T> make_shared(Args...&& args)
    {
        // Though this is the simplist way to do it.
        // The standard version tries to make the whole thing more efficient.
        // by allocating the space for the object T and the ref-count
        // in a single allocation (thus removing the need for an extra
        // call to new).
        return shared_ptr<T>(new T{std::forward<Args>(args)...});
    }

---

    template<typename T>
    T* shared_ptr<T>::operator->() const
    {
        return &*obj_ptr; // Not sure why that is rewquired.
    }

---

This implies you don't keep a reference count object for nullptr. But your object always has a reference counter and you will need to make sure it is updated correctly.

template<typename T>
shared_ptr<T>::~shared_ptr()
{
    if(obj_ptr) // Always call reset.
                // Otherwise you are potentially leaking
                // the reference count object.
        reset();
}

---

Which leads us to the reference count object handling. In reset you need to call delete on the object managed and the reference count object if the count reaches zero.

template<typename T>
void shared_ptr<T>::reset()
{
    obj_ptr = nullptr;

    if(ref_cnt)
        --ref_cnt->use_count_;

    if(ref_cnt->use_count_ == 0)
        // This is not correct.
        // You have called the destructor but leaked the object space.
        ref_cnt->object_->~T();

        // You need to delete the object and the memory used by the
        // reference count object
        // delete ref_cnt->object_;
        // delete ref_cnt;
}

---

Self Plug: Wrote a lot about shared pointers here:

https://lokiastari.com/series/

Read three articles on shared pointers.

Answer 2

1. I am not sure how it works at all as it shouldn't compile. Generic templates require the whole implementation in .hpp/.h files and nothing hidden in .cpp files aside from some possible instantiations.

2. You have a ton of memory leaking due to ref_cnt{new RefCount()} in lots of places.

3. Those are incorrect ways to write copy assignment/operator:

        shared_ptr(shared_ptr& ptr);
        shared_ptr operator=(shared_ptr& ptr);
   

   They should be

       shared_ptr(const shared_ptr& ptr);
       shared_ptr operator=(const shared_ptr& ptr);
   

4. Also, copy-assignment and move-assignment aren't safe for self assignment. E.g., shared_ptr<T> x = ...; x = x; results in a bug.

5. make_shared is supposed to obtain a bunch of arguments and construct the object with the parameters. As it is, it can only construct copy-constructible objects and it isn't what people want with it. It supposed to go like this

      template<typename T, typename... Args>
      shared_ptr<T> make_shared(Args&&... args)
      {
           return shared_ptr<T>(new T(std::forwat<Args>(args)...));
      }
   

   also, std::make_shared saves on the extra allocation of the control block by creating the object and the reference counter together.

6. The reference counter class you use RefCount is not thread-safe. The counter is supposed to be atomic. You cannot use this smart pointer in a multi-thread environment safely which is the primary purpose of the std::shared_ptr.

7. I believe both * and -> operators are supposed to return T& and not T or T*.

8. operator bool() is ought to be explicit else you can made odd comparisons.

9. As it is written following line shouldn't compile shared_ptr<int> x = {}; because it has no default constructor and instead it uses explicit shared_ptr(T* ptr = nullptr); which is explicit.

10. You should consider what to do when T is a const object. At times people want to pass shared_ptr<const T> to some code because they want to make sure that the object isn't modified.

11. You need some more consideration to the object oriented scenario. Frequently people want to create a Derived class and return shared_ptr<Base> and your class has no support for it. Also you have no support for dynamic casting.

Answer 3

I will add my 2 cents to the list.

    T* obj_ptr;
    struct RefCount {
        size_t use_count_;
        T* object_;
    }* ref_cnt;

Here, you are duplicating T* pointer. It would be better to remove RefCount struct and just keep T* and a size_t* member. The benefit would be lower memory usage in the heap.

    T* obj_ptr;
    size_t* use_count;

Some might suggest you to remove the obj_ptr member (which is also a good way) but the 2-pointers solution will greatly ease my next point.

This constructor and copy operator don't allow copies from shared_ptr of child class of T.

    shared_ptr(shared_ptr& ptr);
    shared_ptr operator=(shared_ptr& ptr);

You may want to allow it with:

    template<class U>
    shared_ptr(const shared_ptr<U>& ptr);
    template<class U>
    shared_ptr operator=(const shared_ptr<U>& ptr);