Smart Pointers with Reference Counting - C++

Question

I am trying to improve my general skills in C++ so I plan to write versions of shared_ptr and unique_ptr from scratch. I wrote a version of shared_ptr with reference counting and I was wondering for reviews.

#ifndef __SHIVA_XSMARTPTRS__
#define __SHIVA_XSMARTPTRS__
/*
This tries to define a reference counted smart pointer class instead of
regular pointers
*/
namespace Shiva
{
template <class T>
class SmartPtr
{
 public:
 //Default Constructor
 explicit SmartPtr(T* p = 0)
 {
 if(p)
 ptrWrapper_ = new PointerWrapper(p,1);
 else
 ptrWrapper_ = new PointerWrapper();
 }
 //The copy constructor
 SmartPtr(const SmartPtr& copyObj)
 {
 ptrWrapper_ = copyObj->ptrWrapper_;
 ptrWrapper_->incrementRefCount();
 }
 //Assignment operator
 SmartPtr& operator=(const SmartPtr& copyObj)
 {
 if(this != copyObj)
 {
 //Decrement and decimate as needed
 unsigned int preReleaseRefCount = ptrWrapper_->getRefCount();
 ptrWrapper_->decrementRefCount();
 if(preReleaseRefCount==1)
 {
 delete ptrWrapper_;
 ptrWrapper_ = 0;
 }
 ptrWrapper_ = copyObj->ptrWrapper_;
 ptrWrapper_->incrementRefCount();
 }
 return *this;
 }
 //The main object destructor
 //Essentially decrement the refcount by 1. If the refcount is equal to 0, then you should delete the pointer
 ~SmartPtr()
 {
 unsigned int preReleaseRefCount = ptrWrapper_->getRefCount();
 ptrWrapper_->decrementRefCount();
 if(preReleaseRefCount==1)
 {
 delete ptrWrapper_;
 ptrWrapper_ = 0;
 }
 }
 //Referncing operator
 T& operator*()
 {
 return ptrWrapper_->getReference();
 }
 //Address operator
 T* operator->()
 {
 return ptrWrapper_->getPointer();
 }
 //Get reference counter
 int getRefCount() const
 {
 return ptrWrapper_->getRefCount();
 }
 //Is this the only copy of the object present
 bool unique() const
 {
 return ptrWrapper_->isUnique();
 }
private:
 class PointerWrapper
 {
 public:
 PointerWrapper(T* ptr=0, int refCount = 0)
 :ptr_(ptr),refCount_(refCount)
 {}
 T* getPointer()
 {
 return ptr_;
 }
 T& getReference()
 {
 return *ptr_;
 }
 unsigned int getRefCount()
 {
 return refCount_;
 }
 void decrementRefCount()
 {
 refCount_--;
 if(!refCount_)
 {
 delete ptr_;
 ptr_ = 0;
 }
 }
 void incrementRefCount()
 {
 refCount_++;
 }
 bool isUnique()
 {
 return (refCount_==1);
 }
 private:
 T* ptr_;
 int refCount_;
 };
 PointerWrapper* ptrWrapper_;
};
}
#endif

• Incomputable: Please read the first two points of this answer.
• Loki Astari: IT's a lot harder than you think.

Answer 1

Review

Issue

My first issues is that using your shared pointer is not safe. Once I give you the pointer. You are responsible for deleting it all situations (especially when exceptions happen).

But if the call to new PointerWrapper(p, 1) fails. ie it throws BadAlloc then you will leak my object. So you need to watch out for that allocation and delete the pointer immediately if you throw.

Missing Feature

You have not implemented move semantics. This is absolutely essential.

Your object is missing a swap function.

Design

I don't think the class PointerWrapper buys you ay functionality or makes the code any easier to write.

nullptr

Prefer to use nullptr over 0 as the non existing pointer. 0 is a number and can easily be converted to other things when you least expect it. nullptr of std::nullptr_t and can only be assigned to pointers and is not auto converted into other things.

General grumbles

I think the assignment operator is overly complex. It would be much simpler to write this as using the standard copy and swap idiom.

 SmartPtr& operator=(const SmartPtr& copyObj)
 {
 SmartPtr copy(copyObj);
 copy.swap(*this);
 return *this;
 }

This logic seems a bit overly complex.

 unsigned int preReleaseRefCount = ptrWrapper_->getRefCount();
 ptrWrapper_->decrementRefCount();
 if(preReleaseRefCount==1)
 {
 delete ptrWrapper_;
 ptrWrapper_ = 0;
 }

Personally I would have got decrementRefCount() to return true if it deleted the object.

 if (ptrWrapper_->decrementRefCount())
 {
 delete ptrWrapper_;
 }

No point in setting the ptrWrapper_ to nullptr

 ptrWrapper_ = 0;

When the very next statement sets it to another value.

 ptrWrapper_ = copyObj->ptrWrapper_;

Self Plug

These articles are a good read (I wrote them):

Unique Ptr
Shared Ptr
Constructor