Reference-counting smart pointer class

Question

I made a reference-counting smart pointer class. My aim is to make a "minimal" but "general purpose" smart pointer class with proper documentation. This is basically for educational purposes.

I would like to have comments regarding exception handling, code clarity, comments, ease of use of API, etc. And, also comment if the code is of "industrial level".

#ifndef SMARTPTR_HPP
#define SMARTPTR_HPP

#include <algorithm>

namespace smartptrnamespace
{

//
// Basic reference counting smart pointer (no overloaded Bool magic, etc).
// Ownership of memory on heap is shared amongst objects of this
// class (shared ownership) i.e., no new object of T type is created
// by this class.
// Objects of this class are:
//  1. nothrow-copy-assignable
//  2. nothrow-copy-constructible
//  3. nothrow-destructible, if T is nothrow-destructible
//  4. suitable for storage in a STL container like List, Deque etc.
// Strong exception guarantee
//
// Sample Usage:
//  SmartPtr<T> sPtr1 (new T);  // sPtr1 refers to object of T type
//  SmartPtr<T> sPtr2;          // sPtr2 refers to no object
//  if (!sPtr2.isAssigned ())   // SmartPtr::isAssigned() returns false if no reference is being held
//      cout << "sPtr2 is not holding any reference" << endl;
//  sPtr2 = new T;              // make a new oject of T and pass ownership to sPtr
//
template<class T>
class SmartPtr
{
    public:

        // create a new object which is not
        // refering to any object on heap.
        // no-throw guarantee
        explicit SmartPtr ()
            :   m_pT (NULL),
                m_pRefCount (NULL)
        {
        }

        // new object will point to a memory location on heap given by 'pObj'
        // Strong exception guarantee
        explicit SmartPtr (T *pObj)
            :   m_pT (pObj),
                m_pRefCount (NULL)
        {
            try
            {
                m_pRefCount = new int;
            }
            catch (...)
            {
                checkedDelete (m_pT);
                throw;
            }

            *m_pRefCount = 1;
        }

        // new object will refer to same memory on heap as 'rObj'
        // no-throw guarantee
        SmartPtr (const SmartPtr<T> &rObj)
            :   m_pT(rObj.m_pT), 
                m_pRefCount(rObj.m_pRefCount) 
        { 
            if (m_pRefCount != NULL)
                (*m_pRefCount)++;
        }


        // make 'rObj' and 'this' will refer to same object on heap
        // no-throw guarantee
        SmartPtr& operator= (const SmartPtr<T> &rObj)
        {
            // uses copy-and-swap idiom
            this_type(rObj).swap(*this);
            return *this;
        }

        // assign this smart pointer to another object on heap
        // Strong exception guarantee
        SmartPtr& operator= (T *pTObj)
        {
            // try and setup memory for reference counter
            int *pNewRefCount;
            try                             
            {
               pNewRefCount = new int;
            }
            catch (...)
            {
                delete pTObj;
                throw;
            }

            // stop referring to previous object
            updateCountAndTriggerDelete ();

            // start referring to new object
            m_pRefCount = pNewRefCount;
            *m_pRefCount = 1;

            m_pT = pTObj;

            return *this;
        }

        // no-throw guarantee
        ~SmartPtr ()
        {
            updateCountAndTriggerDelete ();
        }

        // returns true if this object is holding a reference
        // to an object on heap
        // no-throw guarantee
        bool isAssigned()
        {
            return !(m_pT == NULL);
        }

        // no-throw guarantee
        T* operator->()
        {
            return m_pT;
        }

        // no-throw guarantee
        T& operator*()
        {
            return *m_pT;
        }

    private:

        // make sure we dont delete a incomplete type pointer
        // no-throw guarantee
        template <class S>
        void checkedDelete (S* pSObj)
        {
            typedef char type_must_be_complete[ sizeof(S)? 1: -1 ];
            (void) sizeof(type_must_be_complete);
            delete pSObj;     
        }

        // count the references and delete it if this is last reference
        // no-throw guarantee
        void updateCountAndTriggerDelete ()
        {
            if (m_pRefCount != NULL)
            {                  
                (*m_pRefCount)--;

                // if this is last reference delete the memory
                if (*m_pRefCount == 0)
                {
                    checkedDelete (m_pRefCount);
                    checkedDelete (m_pT);
                }
            }
        }

        // swap the pointer values of 'rObj' with values of 'this'
        // no-throw guarantee
        void swap (SmartPtr<T> &rObj)
        {
            std::swap (m_pT, rObj.m_pT);
            std::swap (m_pRefCount, rObj.m_pRefCount);
        }

        // pointer to memory location of object
        T *m_pT;

        // pointer to memory location where 'reference' count of
        // object pointed to by m_pT is kept
        int *m_pRefCount;

        typedef SmartPtr<T> this_type;
};

} // namespace smartptrnamespace

#endif // SMARTPTR_HPP

Answer 1

A few non-expert comments:

• I would prefer the private vars to be at the top of the class - to avoid the immediate need to scroll to the bottom.

• Is there and advantage in making m_pRefCount a pointer to int instead of a simple int?

• Why would you need to use checkedDelete to delete a pointer to int that you created yourself? If you don't use checkedDelete for your local int, then the function can take template type T not a new class S. Is there not a better way to check for an incomplete type? And (showing my ignorance) is it necessary?

Answer 2

1-- Why not nullptr in place of NULL ?

2-- Personally I like if (m_pRefCount) more that if (m_pRefCount != NULL)

3-- In your SmartPtr& operator= (T *pTObj) you delete pTObj; but in your explicit SmartPtr (T *pObj) you checkedDelete (m_pT); Why this inconsistence?

4-- Implementing manually SmartPtr& operator= (const SmartPtr<T> &rObj) could be a litter more efficient, but I find very good your use of the copy-swap: it allow not to duplicate code. Why not use it for;

    SmartPtr& operator= (T *pTObj)
    {
        If (pTObj== m_pT) return *this;     // this may be a litter better.
       // uses copy-and-swap idiom
        this_type(pTObj).swap(*this);    // temporary SmartPtr get deleted, and m_pRefCount decreased (after swap: for the old value).
        return *this;
    }

In this case the only place where updateCountAndTriggerDelete (); will be called is the destructor, and you could move it entirely code there, with seems more natural for me. That also mean that you will don’t need to zeroes any deleted pointer.

~SmartPtr ()
{
       if ( ! m_pRefCount) return;

        if ( 0== --(*m_pRefCount )  )     //     :-)     if this is last reference delete the memory
        {
            delete m_pRefCount;
            checkedDelete (m_pT);
        }
}

5-- Also (from your answer to comments) this SmartPtr& operator= (T *pTObj) is not used during initialization:

T *oldPtr = new T();

SmartPtr<T> sPtr = oldPtr;   // will not compile. Here you have: SmartPtr sPtr ( oldPtr);    but you declared this constructor explicit.
The use of this will be:
SmartPtr<T> sPtr;
sPtr = oldPtr;   
sPtr = oldPtr;   // second time you have a problem if not insert the If (pTObj== m_pT). But this may be OK.

The construction T a=b; is a short for T a(b); and the operator=() is only used to assign a value to an already constructed object, and not to construct a new object and initialize it.

6-- As others said, add const where possible.

Answer 3

1. API should resemble API of existing shared pointers, like std::(tr1::)shared_ptr. They have widely known well-designed API and there is no need to invent something new without a good reason.

2. Pointer should have an operator bool, instead of isAssigned and it should be const.
   In C++ it will be explicit operator bool() const, in C++03 - safe-bool idiom.

3. Those try-catch blocks are awful. You should write unique_ptr first, and use it to temporally own the object:

   explicit SmartPtr (T *pObj) : m_pT(pObj), m_pRefCount()
   {
       unique_ptr<T> temp(pObj);
       m_pRefCount = new int(1);
       temp.release();
   }
   

4. Assignment operator can take argument by value:

   void operator= (SmartPtr<T> rObj) { swap(rObj); }
   

5. Both copy ctor an assignment operator should be templates to allow code like

   struct Derived : Base {...};        
   SmartPtr<Derived> d = ...;
   SmartPtr<Base> b = d;
   

6. operator= (T *pTObj) should use SmartPtr (T *pObj) ctor and dtor

   void operator= (T *pTObj) { SmartPtr<T>(pTObj).swap(*this); }
   

7. SmartPtr<T>(NULL) is broken.

8. You can write typedef SmartPtr this_type; (without <T>)