Yet another smart pointer implementation for learning

Question

I based this implementation on CountedPtr in The C++ Standard Library book by Nicolai Josuttis (page 222) and also available online here.

I know that I can use the C++11 smart pointers but this is a learning exercise and also for some projects an earlier compiler must be used.

I didn't understand the need for the actual reference counter to be a pointer so I just used a standard integer. Maybe that introduces a bug, not sure.

My main concern is about the object lifecycle. Does the reference counter as a straight integer cause a bug? Any feedback will be much appreciated.

Here is the smart pointer class:

#ifndef SMARTPOINTER_HPP__
#define SMARTPOINTER_HPP__

#include <iostream>

template<typename T>
class sp {
public:
   sp(T* ptr) : ptr_(ptr), ref_cnt_(1) {
      std::cout << "sp ctor: " << ptr_ << ", ref_cnt_: " << ref_cnt_ << std::endl;
   }

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

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

   sp(const sp<T>& rhs) : ptr_(rhs.ptr_), ref_cnt_(rhs.ref_cnt_) {
      ++ref_cnt_;
      std::cout << "sp copy ctor: " << ptr_ << ", ref_cnt_: " << ref_cnt_ << std::endl;
   }

   sp& operator=(const sp& rhs) {
      if(this != &rhs) {
         *this = rhs;
         ++ref_cnt_;
         std::cout << "sp assignment: " << ptr_ << ", ref_cnt_: " << ref_cnt_ << std::endl;
      }
      return *this;
   }

   ~sp() { 
     if(ptr_) 
       std::cout << "~sp: " << ptr_ << ", ref count: " << ref_cnt_ << std::endl;
       --ref_cnt_;
       if(ref_cnt_ == 0)
         delete ptr_; 
   }

   bool operator==(const T& that) const { return ptr_ == that.ptr_; }
   bool operator==(T* that) const { return ptr_ == that; }

   bool operator!=(const sp& that) const { return ptr_ != that.ptr_; }
   bool operator!=(sp* that) const { return ptr_ != that; }


private:
   T* ptr_;
   unsigned ref_cnt_;
};


#endif //SMARTPOINTER_HPP__

Here is my code to exercise:

#include <iostream>
#include <string.h>

#include "smart_pointer.hpp"

class name {
public:
   name(const char* label) : label_(label) {}

   const char* get_name() const { return label_; }
   unsigned length() const { return strlen(label_); }

private:
  const char* label_;
};

int main() {

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

      sp<int> myp2(myptr1);

      sp<int> myp3(myptr1);

      sp<int> myp4 = myptr1;
   }


   {
     std::cout << "Test with name object\n";
     sp<name> myname = new name("Andrew");
     std::cout << myname->get_name() << " has length: " << myname->length() << std::endl;
     sp<name> myname2(myname); 
   }

   return 0;
}

I get this output:

Test with ints
sp ctor: 004C7A80, ref_cnt_: 1
number: 4
sp copy ctor: 004C7A80, ref_cnt_: 2
sp copy ctor: 004C7A80, ref_cnt_: 2
sp copy ctor: 004C7A80, ref_cnt_: 2
~sp: 004C7A80, ref count: 2
~sp: 004C7A80, ref count: 2
~sp: 004C7A80, ref count: 2
~sp: 004C7A80, ref count: 1
Test with name object
sp ctor: 004C7A80, ref_cnt_: 1
Andrew has length: 6
sp copy ctor: 004C7A80, ref_cnt_: 2
~sp: 004C7A80, ref count: 2
~sp: 004C7A80, ref count: 1

Answer 1

Main problem with the reference counter:

As it stands right now, your code will eventually leak or try to delete the same pointer more than once. You are keeping a diferent reference count inside each smart pointer, which is not synchronised, so smart pointers sharing the same object can have different counts for the same memory. Take this simple example, similar to your second one:

sp<MyObject> ptr1 = new MyObject(); // ptr1.ref_cnt = 1
sp<MyObject> ptr2 = ptr1; // ptr1.ref_cnt = 1, ptr2.ref_cnt = 2
// Now we have two different counts for the same object!

Once you pass the copy constructor or assignment operator, the reference count of the original pointer is left unaltered. This cannot possibly end well. That's why in the original example you've linked the author used a pointer for the reference counter as well, so that any smart pointer sharing ownership of an object can also point to the same counter. Once the owned object is to be freed, the last pointer also frees the counter.

One cleaver trick to avoid allocating the counter in a separate new call is to allocate both the object and the counter as the same block of raw bytes, then adjust the pointers and construct the object on the remaining memory. This is how some implementations of std::make_shared() work. Once you fix the counter issue, you might want to try this optimization.

Other issues with the code:

• This destructor seems broken:

     ~sp() { 
       if(ptr_) 
         std::cout << "~sp: " << ptr_ << ", ref count: " << ref_cnt_ << std::endl;
         --ref_cnt_;
         if(ref_cnt_ == 0)
           delete ptr_;     
     }
  

  Didn't you forget to wrap the first if with curly braces? This seems like a case where the indenting of the code tries to tell the reader one thing, while the code does a different thing. The first if only applied to the cout call. To avoid this confusion, it is a good idea to always provide { } even for single line control statements. So this is how I believe you intended to write, since it wouldn't make sense decrementing the counter if the pointer is null:

  ~sp() { 
      if(ptr_) {
          std::cout << "~sp: " << ptr_ << ", ref count: " << ref_cnt_ << std::endl;
          --ref_cnt_;
          if(ref_cnt_ == 0) {
              delete ptr_; 
          }
      }
  }
  

• The comparison operators that take a raw pointer should take that pointer by const T*, to make clear that they do not attempt to modify the pointed object.

• You'll probably also want to provide overloads of operator * and -> that are const and return const pointer and reference. Otherwise you aren't able to call those on a const sp<T>.

• Your class still requires a lot of tuning and polishing to be anywhere near usable. If you plan on taking it a step further, I recommend reading Modern C++ Design. The book has a dedicated chapter on Smart Pointers. It is very detailed and well written and I think it is still one of the best references out there on SPs, even though it was released before C++11.

Answer 2

Everything @glampert said.

Other notes:

I didn't understand the need for the actual reference counter to be a pointer so I just used a standard integer. Maybe that introduces a bug, not sure.

Yes that is a bug. All instance of the sahred pointer must share the same count. Otherwise you don't know when to delete the object. In fact you can get multiple objects deleting the same pointer (because each ref count can reach zero at different times). Worse is that one pointer destroys the pointer with delete and other pointers keep using the object.

My main concern is about the object lifecycle. Does the reference counter as a straight integer cause a bug? Any feedback will be much appreciated.

As explained above yes.

To stop allocating space for the object and the counter in two different allocations the standard came up with a function called make_shared<T>() This basically creates the counter and object in the same space (one allocation rather than two). So if you want some practice try doing that.

Move Semantics

Move semantics. C++11 (and C++14) have move semantics. You should definitely start practicing using them.

Comments not covered by @glampert

I think your comparison operators are questionable.

bool operator==(const T& that) const { return ptr_ == that.ptr_; }
//                  ^^^^^That looks like the wrong type. ^^^^^^  T may not have a member ptr
bool operator==(T* that)       const { return ptr_ == that; }
//            ^^^^^ now you are comparing sp to T* is that really useful.
//                  A pointer that has been passed to a shared pointer should no
//                  longer be passed around as a pointer (you have given up ownership
//                  so there should be no passing it around).
//                  If you really want to test the internall pointer use get() to get
//                  the internal pointer and compare that to a pointer.


bool operator!=(const sp& that) const { return ptr_ != that.ptr_; }
// You could also test that the counters are the same object.

bool operator!=(sp* that)       const { return ptr_ != that; }
// Comparing an object with a pointer is never a really useful.
// And should not be made automatic. But here you are comparing a T*
// against an sp* so I think you have another type here.

You should define operator!= in terms of operator== to make sure they are consistent.

Last you never want a pointer to be auto converted into a smart pointer (especially since the compiler created object will usually be temporary and destroy the pointer when it goes out of scope at the end of the expression).

So you should always mark the constructor explicit.

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

Readability:

  if(--*ref_cnt_ == 0) {

That's pretty horrible.
Why not break it into multiple lines:

  unsigned int&  count = *ref_cnt_;
  --count;
  if(count == 0) {

The resulting output by the compiler will be identical. But at least it is readable.

Note: Being good at C++ is not the ability to jam as many commands as possible into the smallest space. A good C++ programmer writes code that is understandable at first glance.

Other code:

Stop using C-Strings.

class name {
public:
   name(const char* label) : label_(label) {}

   const char* get_name() const { return label_; }
   unsigned length() const { return strlen(label_); }

private:
  const char* label_;
};

The addition of std::string will make this code ten times more efficient for the simple reason you will not have to re-compute the length every few iterations.

Getters are a sign of bad design:

const char* get_name() const { return label_; }

Why do you need a getter? When you see a class that has a bunch of getters. That get information from the class do some operation then put a value back into the class it is usally better to put that functionality into the class (as a method).

In your case you only have a getter for printing. So it would be better to have a printing function.

// Assuming you are still using the non std::string method.
class name {
public:
   name(const char* label) : label_(label) {}

   std::ostream print(std::ostream& str) const {
       // You should probably check for NULL
       return str << label_ << " has length: " << strlen(label_);
   }

   // To make print more C++ idiomatic add a friend.
   friend std::ostream& operator<<(std::ostream& str, name const& data) {
       return data.print(str);
   }
private:
  const char* label_;
};

// Now your print is:

int main()
{
    sp<name>   data(new name("Loki"));

    std::cout << *data << "\n";
}

There is rarely ever any need to use std::endl.

std::cout "HI" << std::endl;

The difference between '\n' and std::endl is a flush. Flushing the output manually is very inefficient and one of the main reasons for slower (than C) io code. The standard libraries will flush the output when the buffer is full (ie at the most efficient frequency). Even if you have an interactive application and ask the user questions. Before reading from std::cin the standard libraries will flush std::cout to make sure the user sees the question. So there is never any real reason to manually flush the buffer (don't do it).