shared_ptr basic implementation for non array types

Question

This is an implementation to simulate the basic functionality of c++ shared_ptr. This doesn't provide features like custom deleter and make_shared().

I would really appreciate any feedback to improve the below code, any other api's that I should be providing etc.

#ifndef SHARED_PTR_H_
#define SHARED_PTR_H_

#include <utility>

namespace kapil {
  template <typename T>
  class shared_ptr {
    private:
      T* ptr_;   // data pointer
      int* ref_count_;  // reference count pointer

      void decrement_ref_count_and_delete_if_needed() {
        if (ref_count_) {
          --(*ref_count_);
            if (*ref_count_ == 0) {
              delete ptr_;
              delete ref_count_;
              ptr_ = nullptr;
              ref_count_ = nullptr;
            }
        }
      }

    public:
      constexpr shared_ptr() noexcept : ptr_{nullptr}, ref_count_{nullptr} {}

      constexpr explicit shared_ptr(T* ptr) {                  // constructor
        ptr_ = ptr;
        ref_count_ = new int{1};
      }

      shared_ptr(const shared_ptr& other) noexcept { // copy constructor
        ptr_ = other.ptr_;
        ref_count_ = other.ref_count_;
        if (ref_count_ != nullptr) {
          ++(*ref_count_);
        }
      }

      shared_ptr(shared_ptr&& other) noexcept {  // move constructor
        ptr_ = other.ptr_;
        ref_count_ = other.ref_count_;
        other.ptr_ = nullptr;
        other.ref_count_ = nullptr;
      }

      ~shared_ptr() noexcept {             // destructor
        decrement_ref_count_and_delete_if_needed();
      }

      shared_ptr& operator = (const shared_ptr& other) noexcept {   // assignent operator
        if (this != &other) {
          decrement_ref_count_and_delete_if_needed();
          ptr_ = other.ptr_;
          ref_count_ = other.ref_count_;
          if (ref_count_) {
            ++(*ref_count_);
          }
        }
        return *this;
      }

      shared_ptr& operator = (shared_ptr&& other) noexcept {   // move assignment operator
        *this = other;
      }

      T* get() const noexcept {
        return ptr_;
      }

      void reset() noexcept {
        decrement_ref_count_and_delete_if_needed();
      }

      void reset(T* ptr) {
        decrement_ref_count_and_delete_if_needed();
        ptr_ = ptr;
        if (!ref_count_) {
          ref_count_ = new int{1};
        }
        *ref_count_ = 1;
      }

      int use_count() const noexcept {
        return *ref_count_;
      }

      void swap (shared_ptr& other) {
        std::swap(ptr_, other.ptr_);
        std::swap(ref_count_, other.ref_count_);
      }

      T& operator * () const {
        return *ptr_;
      }

      T* operator -> () const noexcept {
        return ptr_;
      }

      explicit operator bool() const noexcept {
        return (ptr_ != nullptr);
      }

      friend bool operator == (const shared_ptr& lhs, const shared_ptr& rhs) noexcept {
        return (lhs.ptr_ == rhs.ptr_);
      }

      friend bool operator != (const shared_ptr& lhs, const shared_ptr& rhs) noexcept {
        return !(lhs == rhs);
      }
  }; // class shared_ptr

  template <typename T>
  void swap(shared_ptr<T>& lhs, shared_ptr<T>& rhs) {   // swap function in namespace to facilitate ADL
    lhs.swap(rhs);
  }
} // namespace kapil

#endif

Answer 1

It might be difficult to get great feedback on this shared_ptr just because it omits so many of the features of std::shared_ptr. I mean I'd be tempted to just list all the features it doesn't have and then fail to review the code at all:

• Atomic/thread-safe accesses to the reference count.
• Implicit conversion from shared_ptr<T> to shared_ptr<U> whenever T* is convertible to U*.
• shared_ptr<void>.
• Custom and type-erased deleters.
• The "aliasing constructor."
• weak_ptr.
• Implicit conversion from unique_ptr.
• make_shared.

---

~shared_ptr() noexcept {             // destructor

The comment is redundant. And so is the noexcept: in C++11-and-later, destructors are implicitly noexcept, and it's idiomatic not to write it explicitly.

Contrariwise, it is extremely important to provide a noexcept swap function! You're missing the noexcept keyword here:

void swap (shared_ptr& other) {

By the way, it's weird to put a space between the name of the function and its opening parenthesis. Prefer swap(shared_ptr& other).

---

You've got a typo in a comment: assignent. Look for typos elsewhere. Where there's one mistake, there's often more than one. (Also, eliminate that redundant comment.)

---

      --(*ref_count_);
        if (*ref_count_ == 0) {

Your accesses to *ref_count already aren't thread-safe; but FYI, here's where the race condition would sneak in if you just went and made ref_count a pointer to a std::atomic<int>. In a multithreaded environment, it might well be that *ref_count_ == 0, but that doesn't mean that you're the one whose decrement took it to 0.

---

constexpr shared_ptr() noexcept : ptr_{nullptr}, ref_count_{nullptr} {}

If you used C++11 non-static data member initializers (NSDMIs) for ptr_ and ref_count_, you could =default this constructor.

T *ptr_ = nullptr;
int *ref_count_ = nullptr;

constexpr shared_ptr() noexcept = default;

---

shared_ptr& operator = (shared_ptr&& other) noexcept {   // move assignment operator
    *this = other;
}

Why did you bother implementing this overload at all, if you're not going to make it more efficient? Either omit it entirely (so that there's only one assignment operator), or implement it as e.g.

shared_ptr& operator=(shared_ptr&& other) noexcept {
    if (this != &other) {
        decrement_ref_count_and_delete_if_needed();
        ptr_ = std::exchange(other.ptr_, nullptr);
        ref_count_ = std::exchange(other.ref_count_, nullptr);
    }
    return *this;
}

By the way, once you make your swap function noexcept, you might consider saving some brain cells by using the copy-and-swap idiom:

shared_ptr& operator=(shared_ptr&& other) noexcept {
    shared_ptr(std::move(other)).swap(*this);
    return *this;
}

---

  void reset() noexcept {
    decrement_ref_count_and_delete_if_needed();
  }

This is wrong. Consider:

shared_ptr<int> p(new int);
shared_ptr<int> q = p;
p.reset();
assert(p.get() != nullptr);  // oops

---

  void reset(T* ptr) {
    decrement_ref_count_and_delete_if_needed();
    ptr_ = ptr;
    if (!ref_count_) {
      ref_count_ = new int{1};
    }
    *ref_count_ = 1;
  }

This is also wrong. Consider:

shared_ptr<int> p(new int);  // A
shared_ptr<int> q = p;       // B
p.reset(new int);            // C
p = shared_ptr<int>();       // D
assert(q.use_count() == 0);  // oops!

Line A sets the refcount to 1. Line B increments the refcount to 2. Line C repoints p.ptr_ (but does not change p.ref_count_ ��� oops!), and then resets the refcount to 1. Line D decrements the refcount to 0 and frees the int that was allocated on line C. Now q.ptr_ still points to the int that was allocated on line A, but *q.ref_count_ == 0.

The correct implementation of reset(T*) is simply

  void reset(T *ptr) {
      *this = shared_ptr(ptr);
  }

(I strongly advise against ever making calls to reset from user code, by the way, because every call to reset can be more readably expressed as a simple and type-safe assignment.)

---

int use_count() const noexcept {
    return *ref_count_;
}

There's an unspoken precondition here: that use_count() shall never be called on a shared_ptr in the default-constructed ("disengaged", "moved-from", "partially formed") state. If you keep this precondition, then the Lakos Rule would suggest that this function shouldn't be noexcept. However, I think it would be more natural to write

int use_count() const noexcept {
    return ref_count_ ? *ref_count_ : 0;
}

so that the function always has well-defined behavior.

Answer 2

Overview

• You can still leak the pointer from the constructor.
• You can crash the program when a T destructor safely throws.
• You ahve some bugs around reset.

Code Review

My biggist issue is here:

  constexpr explicit shared_ptr(T* ptr) {                  // constructor
    ptr_ = ptr;
    ref_count_ = new int{1};
  }

The problem is that new can throw. If it throws during a constructor then the destructor is never run. and thus you will leak ptr. The whole point of the class is to prevent leaking so you need to think about this situation.

The function decrement_ref_count_and_delete_if_needed() is called from several function that are marked as noexcept. So this function should also be noexcept or some of your noexcept functions should not be marked noexcept.

In this function you have to either make a concerted effort to make sure no exceptions propagate or that you don't affect the state of your object if it does throw.

The problem is this line:

          delete ptr_;

Here ptr_ is of type T. You don't know what type T and thus you can not guarantee that it does not throw.

The reset() is broken.

  void reset() noexcept {
    decrement_ref_count_and_delete_if_needed();
  }

If I reset a shared pointer then it should be nullptr inside. This function does not set this object to nullptr (unless it is the only pointer to the object).

The reset(T* ptr) is also broken.

  void reset(T* ptr) {
    decrement_ref_count_and_delete_if_needed();   // decrement ref count
                                                  // but if this was not the
                                                  // only pointer to the object
                                                  // then these value are still
                                                  // pointing at the old values.


    ptr_ = ptr;                                   // Overwrite the pointer OK


    if (!ref_count_) {                            // THIS IS WRONG.
                                                  // If you have a pointer to a count
                                                  // This count belongs to the other
                                                  // pointer you were previously
                                                  // countint for.


      ref_count_ = new int{1};                    // BASICALLY THIS LINE SHOULD
                                                  // ALWAYS BE USED.
    }
    *ref_count_ = 1;                             
  }

Further Reading

I wrote some articles around writting smart pointer you may find useful:

Smart-Pointer - Unique Pointer
Smart-Pointer - Shared Pointer
Smart-Pointer - Constructors