custom smart pointer class template

Question

Kindly provide your review comments:

//SmartPtr.h
class RefCount {
public:
    void AddRef() {
        ++(this->_count);
    }
    int Release() {
        return --(this->_count);
    }
private:
    int _count;
};

template <class T>
class SmartPtr {

public:

    // constructor
    SmartPtr();
    SmartPtr(T* iObject);

    // copy constructor
    SmartPtr(const SmartPtr<T>& iSPtr);

    // destructor
    ~SmartPtr();

    // operators
    SmartPtr<T>& operator=(const SmartPtr<T>& iSPtr);
    T& operator*();
    T* operator->();

private:
    T* _ptr;
    RefCount* _refCount;

    void _release();
    void _copySmartPtr(const SmartPtr<T>& iSPtr);
};

//SmartPtr.cpp
#include "SmartPtr.h"

// constructor
template <class T>
SmartPtr<T>::SmartPtr():
    _refCount(nullptr),
    _ptr(nullptr)
{
}

template <class T>
SmartPtr<T>::SmartPtr(T* iObject):
    _refCount(new RefCount()),
    _ptr(iObject)
{   
    this->_refCount->AddRef();
}

// copy constructor
template <class T>
SmartPtr<T>::SmartPtr(const SmartPtr<T>& iSPtr) 
{
    this->_copySmartPtr(iSPtr);
}

// destructor
template <class T>
SmartPtr<T>::~SmartPtr() {
    this->_release();
}

// operators
template <class T>
SmartPtr<T>& SmartPtr<T>::operator=(const SmartPtr<T>& iSPtr) {
    if (iSPtr._ptr && (this != &iSPtr)) {
        this->_release();
        this->_copySmartPtr(iSPtr);
    }
    return *this;
}

template <class T>
T& SmartPtr<T>::operator*() {
    return *(this->_ptr);
}

template <class T>
T* SmartPtr<T>::operator->() {
    return this->_ptr;
}

template <class T>
void SmartPtr<T>::_release() {
    if (this->_refCount && this->_refCount->Release() == 0) {
        delete this->_ptr;
        delete this->_refCount;
    }
}

template <class T>
void SmartPtr<T>::_copySmartPtr(const SmartPtr<T>& iSPtr) {
    this->_ptr = iSPtr._ptr;
    this->_refCount = iSPtr._refCount;
    this->_refCount->AddRef();
}

Answer 1

The Big Issue

One of the biggest things about smart pointers is that they guarantee deletion of the pointer. EVEN when exceptions happen. My main problem here is that your constructor may leak the pointer if it fails to initialize correctly.

template <class T>
SmartPtr<T>::SmartPtr(T* iObject):
    _refCount(new RefCount()),       // This can throw.
    _ptr(iObject)
{   
    this->_refCount->AddRef();
}

If the creation of the _refCount object fails you will leak the pointer. You need to guarantee that the pointer does not leak:

SmartPtr<T>::SmartPtr(T* iObject)
try                               // Add a try block
    :_refCount(new RefCount())
    ,_ptr(iObject)
{   
    this->_refCount->AddRef();
}
catch(...)
{
    delete iObject;    // If there is an exception make sure the object is deleted.
    throw;             // Then re-throw the exception.
}

Minor Missing Functions

The main missing things I see are:

 T*  get();            // Sometimes its nice to just get the pointer.
 operator bool();      // Nice when you want to simply check the smart
                       // pointer has something in a boolean context.

 SmartPtr<Stuff>  data = getSmartPointer();
 if (data) {
     data->doWork();   // Need to check there is a pointer in the
 }                     // object before calling doWork().

Constructor Problems

Explicit

Your one parameter constructor should be explicit. Think of this situation.

 void doStuff(SmartPtr<Stuff> work)
 {
      if (work) {
          work->doWork();
      }
 }

Looks simple enough. When you call the function you get another reference counted version of the pointer and thus it is simple and safe to use.

But what happens when I do this:

int main()
{
    Stuff*  work = new Stuff;
    doStuff(work);
    work->moreActions();
    delete work;
}

This code compiles. But the call to doStuff() results in delete being called on the work object. Even though you though it was safe to call (as you are making a copy of smart pointer).

The trouble is that the compiler has converted the Stuff* to a SmartPtr which is deleted in this scope.

What About nullptr

Your object does not accept a nullptr!

 SmartPtr<Stuff>  value = nullptr;  // fails to compile.

It does not look like a major problem. But when you start using templatized code where things are initialized and your type can be swapped in then it becomes an issue as your type can not be used.

What About Derived Types.

On of the major things about C++ is derived types with more functionality.

Derived*    x = new Derived;
Base*       y = x;

The same should work with smart pointers.

SmartPtr<Derived>  x = new Derived;
SmartPtr<Base>     y = x;           // fails to compile.

This will be a common in most C++ code (not exactly like this). But this functionality is really needed.

Make Shared

One of the things the standard builders found was that each shared pointer required the allocation of TWO objects. One for the thing and one for the counter.

They remedied this by introducing std::make_shared<T>(). This does one allocation that allocates the object and the counter inside the same space thus reducing the overall overhead of the object.

Further Reading

I cover a lot of these details and more in some articles I wrote:

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

Answer 2

• As @bipll mentioned, templates must be fully available everywhere they are used. Maybe move the implementation to the header, or at least add a note that the .cpp file is intended to be included instead of the header, as this is unusual.

• Some operators (*, ->) need const overloads (or need to be made const), as otherwise the contained pointer isn't be accessed from a const context, e.g. a const SmartPtr<T>&.

• Please provide an easier access to the underlying pointer than smart_ptr.operator->(). There are functions that only accept raw pointers, for which this way of accessing SmartPtr<T>::_ptr gets cumbersome.

• This implementation seems similar to std::shared_ptr in the sense that the SmartPtr is intended to be shared. However, if this "being shared" involves multiple threads, then accesses to RefCount::_count need to be synchronized. (Otherwise you get race conditions.)

  Sadly, C++98 doesn't offer any help for this in the standard library (as the notion of threads only got formally introduced to C++11), so you have to rely on some external library and/or platform dependent operations.

• nullptr only got introduced in C++11, so a C++98 compiler shouldn't be able to compile this.

Answer 3

• There's no need to use this-> to access any of the members of this class. Simply refer to them in the natural way:

            return --_count;
   // not:  return --(this->_count);
  

• The RefCount class doesn't need to be visible outside of the pointer implementation, so it can be a private class within SmartPtr.

• The code has severe problems with concurrency - however, that's hard to deal with portably, before the current memory model and introduction of std::atomic types.

Answer 4

Consider the null case a bit more

If the smart pointer hasn't been assigned, then you have a null pointer by default. That seems fine, but if I call *sptr on an instance, I will get a null reference, which is UB.

This code, then, relies on the caller never calling operator* when the smart pointer is in this (evidently acceptable) state. To check that, the caller will have to write something like:

if(sptr())
   doThingWithReference(*sptr);

I would be inclined to make 2 changes, then: throw if the dereference operator is called when the pointer is null (exceptions are better than UB), and also to define a bool operator:

explicit operator bool() const {
  return _ptr == nullptr;
}

You mention C++98 in a comment; this explicit version is only C++11 (even if you swapped nullptr for 0), prior to that you had to do interesting things to prevent a bool operator making way for int casting and so on.

Assignment to a null smart pointer will add references to nothing

Consider SmartPtr<A> sptr_a = sptr_null: you Release a reference in the sptr_a refcount, then assign the pointer to that in sptr_null (which is a null pointer), and then add 1 to the mutually held nothing.

If you then assign this again to a different smart pointer sptr_a = sptr_b, then the assignment operator will refuse to _release the old pointer (pointing to nothing), and refuse to copy in the new one, too. If you have received a SmartPtr as a return value and assigned it to such a pointer like this:

SmartPtr<A> sptr_a = sptr_null;
sptr_a = GetJewel();

Then sptr_a will still have a null pointer and the jewel will be thrown away.

Initialise your reference count

At present, there is nothing to initialise the reference count itself to zero except some good luck. The compiler should have warned you about that.