Smart pointer mark II - my answer to C++ 11 shared_ptr

Question

I am writing this smart pointer as a learning exercise. Any feedback would be most appreciated.

Any flaws? Have I missed any test cases?

smart_pointer.hpp:

#ifndef SMARTPOINTER_HPP_
#define SMARTPOINTER_HPP_

#include <iostream>

template<typename T>
class sp {
public:
   sp(T* ptr) : ptr_(ptr), ref_cnt_(new unsigned(1)) {
      std::cout << "sp ctor. Address: " << ptr_ << ", ref_cnt_: " << *ref_cnt_ << '\n';
   }

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

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

   // access to raw ptr
   T* get() { return ptr_; }

   sp(const sp<T>& rhs) : ptr_(rhs.ptr_), ref_cnt_(rhs.ref_cnt_) {
      addref();
      std::cout << "sp copy ctor: " << ptr_ << ", ref_cnt_: " << *ref_cnt_ << '\n';
   }

   sp& operator=(const sp& rhs) {
      if(this != &rhs) {
         try_delete();
         ptr_ = rhs.ptr_;
         ref_cnt_ = rhs.ref_cnt_;
         addref();
         std::cout << "sp assignment: " << ptr_ << ", ref_cnt_: " << *ref_cnt_ << '\n';
      }
      return *this;
   }

   ~sp() { 
        std::cout << "~sp. Address: " << ptr_ << ", ref count before remove_ref: " << *ref_cnt_ << '\n';
        try_delete();  
   }

private:
   unsigned addref() {
      return ++*ref_cnt_;
   }
   unsigned remove_ref() {
      return --*ref_cnt_;
   }

   void try_delete() {
      if(remove_ref() == 0) {
         std::cout << "try_delete(): finally deleting ptr_ at address: " << ptr_ << ", ref count: " << *ref_cnt_ << '\n'; 
         delete ref_cnt_;
         delete ptr_;
      }
   }

   T* ptr_;                    // raw ptr
   unsigned* ref_cnt_;  // shared ownership
};

#endif //SMARTPOINTER_HPP_

Code to exercise:

#include <iostream>
#include <vector>

#include "smart_pointer.hpp"

// useless class just used to test with object more complex than int
class tester {
public:
   tester(const int id, const char* name) : id_(id), name_(name) {}

   const char* const get_name() const { return name_; }
   const int get_id() const { return id_; }

private:
   const int id_;
   const char* const name_;
};


// global vector to hold smart pointer
std::vector< sp<int> > gvs;


int main() {

   {
      std::cout << "Test with ints\n";
      sp<int> myptr1 = new int(3);  
      *myptr1 = 4;
      std::cout << "number: " << *myptr1 << '\n';

      {
         // test usage in collection
         std::vector< sp<int> > vs;
         vs.push_back(myptr1);
      }

      // global collection
      gvs.push_back(myptr1);


      // copying
      sp<int> myp2(myptr1);
      sp<int> myp3(myptr1);
      sp<int> myp4 = myptr1;
   }


   {

      std::cout << "Test with more complex objects\n";
      sp<tester> spt = new tester(1, "Keira Knightley");

      tester* p = spt.get();
      std::cout << "Name: " << p->get_name() << ", id: " << p->get_id() << '\n';

      // copying
      sp<tester> spt2 = spt;
      sp<tester> spt3(spt);

      sp<tester> spt4(new tester(2, "Cate Blanchett"));

      // re-assign
      spt = new tester(3, "Natalie Portman");

   }

   return 0;
}

Answer 1

(1) Style nit: Prefer #pragma once to fragile #ifndef-based include guards. #pragma once isn't yet part of the standard; but any compiler that supports C++11 will definitely have supported #pragma once for years already.

(2) Obviously, remove the std::cout usage from your constructors when using this class for real. :)

(3) This is a MAJOR issue, and a common one, which is why I want to make a big deal of it:

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

No no no no no. You should provide only one operator*, and it should look like this:

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

The act of dereferencing a (smart) pointer does not modify the pointer object, and therefore should be a const method of that pointer object. Also, the result of dereferencing a (smart) pointer to T had darn well better be a reference to T! Adding const to the return type is just plain wrong.

This is the same confusion that people often have between int * const and int const *, or between std::vector<int>::iterator const and std::vector<int>::const_iterator. In your case the confusion is between sp<int> const and sp<int const>.

(4) As above, you should have only one operator-> and one get(), and both should be const methods:

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

(5) Your constructor should almost certainly be explicit:

explicit sp(T* ptr) : ptr_(ptr), ref_cnt_(new unsigned(1))

The exception-safety guarantees here look good to me. I would argue that size_t would be more appropriate than unsigned (you're probably paying for a 16-byte allocation anyway, so why not use that space?), but that's a nitpick.

If you really care about memory allocations, and want to keep learning about how the standard shared_ptr does things, you should try to implement the equivalent of std::make_shared for your smart pointer type, so that the memory allocations can be wrapped up together into a single call to new. I'd imagine that the tricky parts of make_shared have to do with alignment requirements.

(6) Your ref_cnt_ doesn't point to a std::atomic<unsigned>, so addref() is not atomic; copying the same sp from two threads at once may result in undefined behavior.

(7) In C++11, it would be advisable to implement a move constructor and a move assignment operator in order to avoid a pointless addref() and removeref() when moving an rvalue. However, there's no problem with correctness in C++11; because you implement a user-defined copy constructor, the compiler will correctly suppress the generation of default move constructor and assignment functions (which would do the Wrong Thing if they were generated, so it's a good thing they're not).

(8) Your copy assignment operator performs unnecessary operations in the case that x is being assigned from a copy of x; for example:

sp<int> x(new int);
sp<int> y = x;  // adds a ref, correctly
x = y;  // removes a ref, then adds a ref; could be implemented as a no-op instead

(9) You don't use remove_ref except inside try_delete. Personally, I would inline the one call to remove_ref, and then rename try_delete to remove_ref. I would also rename addref to add_ref for consistency, and remove its unused return value.

(Future) To continue building your smart pointer into something like the standard's std::shared_ptr, consider the following use-cases:

sp<int> s(nullptr);  // or (int *)NULL
s = nullptr;
s.reset();  // same as =nullptr

Consider inheritance, upcasting and downcasting. This will require fleshing out your ref_cnt_ pointer from unsigned* to struct ref_cnt*, and careful consideration of what metadata you need to keep in there.

class Mother { int m; };
class Father { int f; };
class Child : public Mother, public Father { int c; };

sp<Child> ch(new Child);
sp<Father> fa = ch;  // casting derived to base should work
ch.reset();
fa.reset();  // this should call ~Child on the correct pointer

Consider providing "weak references", which the standard library provides as std::weak_ptr. Weak references (weak_ptr) and strong references (shared_ptr) work tightly together, using the same infrastructure.

Consider providing the ability to pass in a custom deleter:

sp<char> buf(malloc(1024), (void(char*))free);
buf.reset();  // this should call free on the correct pointer