Attempt at Smart Pointer Implementation

Question

Here is a smart pointer that "knows" the number of existing instances:

smart_ptr.h:

#ifndef _SMART_PTR_H_
#define _SMART_PTR_H_
/*
 Keeps track of the number of instantiated pointers 
*/
class Reference_counter
{
protected:
 typedef unsigned int ValueType;
public:
 Reference_counter () : count(0) { } // default constructor 
 explicit Reference_counter (ValueType c) : count(c) { } // constructor
 Reference_counter (const Reference_counter& rc); // copy constructor
 Reference_counter& operator= (const Reference_counter& rc); // copy assignment
 ~Reference_counter () { } // destructor
 // modifying member functions
 void add_reference () { ++count; } // increments counter, reflecting new pointer creation
 bool release (); // decrements counter and check if any pointers left
private:
 ValueType count; // counts the number of pointers 
};
//----------------------------------------------------------------------------------------------- 
/* 
 Allows multiple pointers to single object. If number of pointers pointing to the same object zero, object deleted.
*/
template <class T>
class Counted_ptr 
{
protected:
 typedef T* PointerType;
 typedef T& ReferenceType;
public:
 Counted_ptr(); // default constructor
 explicit Counted_ptr(PointerType v); // constructor
 Counted_ptr(Counted_ptr<T>& c_ptr); // copy constructor 
 Counted_ptr<T>& operator= (Counted_ptr<T>& c_ptr); // copy assignment 
 ~Counted_ptr(); // destructor
 // modifying member functions
 PointerType get() { return value; } // pointee object value access 
 void reset(PointerType v); // pointer reassignment
 PointerType release(); // pointer resources transfer
 // access operators
 ReferenceType operator* () const { return *value; } 
 PointerType operator-> () const { return value; }
protected:
 void Destroy() { delete value; } // destroy pointee object and free its memory
 static PointerType Default() { return nullptr; } // default value for the class StoredType
private:
 PointerType value;
 Reference_counter* refCount; // If counter = 0; the pointee object is deleted.
};
#include "smart_ptr_def.cpp"
#endif

smart_ptr_def.cpp:

template<class T>
Reference_counter& Reference_counter::operator= (const Reference_counter& rc) 
{
 if (this == &rc)
 {
 return *this;
 }
 count = rc.count;
 return *this;
}
//----------------------------------------------------------------------------------------------- 
template<class T>
bool Reference_counter::release () 
{
 if (!--count) 
 {
 return true; 
 }
 return false;
}
//----------------------------------------------------------------------------------------------- 
template<class T>
Counted_ptr<T>::Counted_ptr() 
 : value(Default()), refCount(nullptr)
{
 reCount = new Reference_counter()
 refCount->add_reference();
}
//----------------------------------------------------------------------------------------------- 
template<class T>
Counted_ptr<T>::Counted_ptr(PointerType v) 
 : value(v), refCount(nullptr) 
{
 refCount = new Reference_counter();
 refCount->add_reference();
}
//----------------------------------------------------------------------------------------------- 
template<class T>
Counted_ptr<T>::Counted_ptr(Counted_ptr<T>& c_ptr) 
 : value(c_ptr.value), refCount(c_ptr.refCount)
{
 refCount->add_reference();
}
//----------------------------------------------------------------------------------------------- 
template<class T>
Counted_ptr<T>& Counted_ptr<T>::operator= (Counted_ptr<T>& src)
{ 
 if (this == &src)
 {
 return *this;
 }
 if (refCount->release())
 {
 Destroy();
 delete refCount; 
 }
 value = c_ptr.value;
 refCount = c_ptr.refCount;
 refCount->add_reference();
 return *this;
}
//-----------------------------------------------------------------------------------------------
template <class T>
Counted_ptr<T>::~Counted_ptr() 
{
 if (refCount->release())
 {
 Destroy();
 delete refCount;
 }
}
//-----------------------------------------------------------------------------------------------
template <class T>
void Counted_ptr<T>::reset(PointerType v) 
{
 if (value)
 {
 Destroy();
 }
 value = v;
} 
//-----------------------------------------------------------------------------------------------
template <class T>
T* Counted_ptr<T>::release()
{
 PointerType temp = value;
 value = Default();
 return temp;
}

main:

#include <iostream>
#include "smart_ptr.h"
int main() 
{
 Counted_ptr<int> p(new int);
 *p.get() = 5;
 std::cout <<"p points to: "<< *p << "\n";
 int *temp = p.release();
 std::cout <<"caller of release(), temp points to: "<< *temp << "\n";
 Counted_ptr<int> p2(p);
 std::cout <<"copy constructed p2 points to: "<< *p2 << "\n";
 Counted_ptr<int> p3(new int(20));
 p3 = p2;
 std::cout <<"copy assigned p3 points to: "<< *p3 << "\n";
}

Questions:

1. Are there any obvious errors that need to be fixed?
2. Is the design and structure of the implemented classes good enough?
3. What additional feature / functionality (that the generic smart pointer provides) could be added to the above code?

Answer 1

Don't include "*.cpp" files inside "*.h" files.

// smart_ptr.h
#include "smart_ptr_def.cpp"

When you separate the template definition from declaration code the definition usually goes in a "*.tpp" file. (By usually read a common convention but by no means ubiquitous).

Continuing with the naming theme. The name of the file usually mirrors the name of the main class being defined. Yours don't match at all. Your file is smart_ptr.h but the class is Counted_ptr.

Missing Functionality

Move semantics. I suppose its not a requirement but sometimes you want to move a shared pointer.

 std::shared_ptr<int> data = assignValue();
 doWork(std::move(data));
 // data contains `nullptr`

Casting to bool. It is useful to use a shared pointer in a boolean context and thus allow you to test its state naturally.

 std::shared_ptr<int> data = assignValue();
 if (data) {
 // data is not `nullptr` and we can do work here.
 }

Moving pointers between different shared pointers.

 class X {};
 class Y: public X {};
 class Z: public X {};
 std::shared_ptr<Z> dataZ(new Z);
 std::shared_ptr<X> dataX(dataZ);

Even though Z inherits from X the shared pointers don't have the same property. shared_ptr<X> is not related to shared_ptr<Z> but we want to be able to use them as if there was a hierarchy. Just like you would if there was a pointer.

With nullptr being the expected pointer being used for null (or no object). You should be able to explicitly use this. Trouble is that you can't because nullptr has a type of nullptr_t which does not match any of your constructors.

Interface Review

Identifiers with a leading underscore are a bad idea. The rules around a leading underscore are complex so best to just completely avoid (even if you know all the rules people reading your code probably don't).

#ifndef _SMART_PTR_H_
#define _SMART_PTR_H_

Here these identifiers are reserved for the implementation.

I question your need to copy a Reference_counter.

 Reference_counter (const Reference_counter& rc);
 Reference_counter& operator= (const Reference_counter& rc);

Even creating a non zero reference counter seems a bit suspect.

 explicit Reference_counter (ValueType c) : count(c) { }

If you can create a non zero reference counter then you can combine this with the default constructor.

What is your use case for this assignment operator?

 Counted_ptr<T>& operator= (Counted_ptr<T>& c_ptr);

As you are returning a pointer the get() method can be const and still return a mutable pointer.

 PointerType get() const { return value; }
 ^^^^^

You already use const on access the underlying object here:

 ReferenceType operator* () const { return *value; } 
 PointerType operator-> () const { return value; }

Code Review

When writing the assignment operator it is usually better to use the copy and swap idiom. Note: In this situation the test for self assignment is worthless and only slows the code down.

template<class T>
Reference_counter& Reference_counter::operator= (const Reference_counter& rc) 
{
 if (this == &rc)
 {
 return *this;
 }
 count = rc.count;
 return *this;
}

Write readable code:

template<class T>
bool Reference_counter::release () 
{
 if (!--count) 
 {
 return true; 
 }
 return false;
}

This is not as readable:

 if (!--count) 

as:

 --count
 if (!count)

Also prefer not to use if then else when all you are doing is returning true/false. Just return the value you were testing.

 --count;
 return count;

Prefer to use initializer list rather code in the constructor.

template<class T>
Counted_ptr<T>::Counted_ptr() 
 : value(Default()), refCount(nullptr)
{
 reCount = new Reference_counter()
 refCount->add_reference();
}

Major FAIL. Once you have handed a pointer to the constructor of a shared pointer you own that pointer. You must delete the pointer when this object is destroyed. Normally the destructor will do this. But if you fail inside the constructor then the destructor will not be called.

template<class T>
Counted_ptr<T>::Counted_ptr(PointerType v) 
 : value(v), refCount(nullptr) 
{
 refCount = new Reference_counter();
 refCount->add_reference();
}

Thus you will leak the pointer here if the call to new Reference_counter() throws an exception. In this situation you must catch delete the pointer and re-throw the exception.

Another Fail. The problem here is that if your call to Destroy() fails (with an exception (yes rare but it can happen)). You now have a shared pointer containing an invalid pointer (and a count of zero). This may work but I am not sure it works in all situations.

template<class T>
Counted_ptr<T>& Counted_ptr<T>::operator= (Counted_ptr<T>& src)
{ 
 if (refCount->release())
 {
 Destroy();
 delete refCount; 
 }
 value = c_ptr.value;
 refCount = c_ptr.refCount;
 refCount->add_reference();
 return *this;
}

If you use the standard copy idiom this problem is automatically solved.

This is Wrong.

template <class T>
void Counted_ptr<T>::reset(PointerType v) 
{
 if (value)
 {
 Destroy();
 }
 value = v;
} 

Like other situations you need to check the reference count. You are not changing all the other shared pointers if you change this one (if this is your use case then you need to call it something else and give some good documentation).

There is a reason that std::shared_ptr does not have the methods release(). If you release a pointer from a shared pointer then all shared pointers need to reset there pointer to nullptr so that the shared pointer will never delete it.

template <class T>
T* Counted_ptr<T>::release()
{
 PointerType temp = value;
 value = Default();
 return temp;
}

Currently you have only reset the value of the current Counted_ptr object. If you have a bunch of shared pointers sharing then all of these others still have the same pointer and you will have a potential for a leak as the order of destruction will now determine if the object is actually deleted.

Self Promo:

Articles I wrote on the object:

• Unique Ptr
• Shared Ptr
• Constructors

• Ziezi: Thanks for the extensive, constructive review and the additional, very enlightening materials! I've added move constructor and assignment (through custom swap), as well as explicit bool to check validity of pointer (but prevent comparison through conversion). Now, for the derived type assignment I directly read your article, but couldn't figure out how to implement release() in my case (for shared_ptr) so that the pointer value and the counter are transferred correctly, especially considering the Interface Review.
• Ziezi: Lastly I used copy and swap idiom and considered the possiblebad_alloc by new in the constructor - by deleting value and re-throw-ing bad_alloc. I now understand why the last two methods release() and reset() work only with unique_ptr. P.S.: also added a constructor with nullptr_t argument, as I use explicit in the other one, which would prevent type conversion and use of nullptr assignments.