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

• Offirmo: I'm not sure... but it seems your copy operators don't handle self assignment (cf. Effective C++ item 11)
• Offirmo: I would also add an exception throw if someone try to dereference a NULL smart pointer. In my opinion it would be useful.
• Offirmo: I would rename updateCountAndTriggerDelete to decreaseCountAndTriggerDelete
• Offirmo: Also, your smart pointer is not thread-safe.
• Offirmo: You can add const in a lot of places : params, isAssigned(). That would make your code more robust and able to work in more situation.

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?

• Corbin: It has to be a pointer or otherwise how would multiple instances of the class access the same int? SharedPtr b = a; b needs to increment a's counter too. In this case, it's simple to just go inside of a and increment it, but in other cases (such as when a falls out of scope, but b doesn't) simple manipulation like that isn't possible.
• Abyx: in c++, we usually put public members at the top.
• Loki Astari: @Abyx: Don't agree with that. I would also have put the private member variables at the top to aid in readability. I would put the private member functions at the bottom.
• Abyx: @LokiAstari better readability for people who use you class?
• Loki Astari: @Abyx: For people that maintain the class. The most important users.

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.

• Abyx: obviously it's C++03 code, that's why no nullptr

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>)