C++: Smart Pointer Implementation

Question

Please could somebody verify that my method of implementing Smart Pointer is correct, and if there is maybe a more efficient way of achieving it.

#ifndef SHARED_PTR
#define SHARED_PTR
#include <iostream>
#include <mutex>
using namespace std;
template <class T>
class RCObject
{
 T* rawObj;
 unsigned int count;
 mutex m_Mutx;
public:
 RCObject() :rawObj(nullptr), count(0){}
 ~RCObject()
 {
 deref();
 }
 RCObject(T* _rawObj):rawObj(_rawObj)
 {
 count = 0;
 }
 int getCount() const 
 {
 return count;
 }
 T* get()
 {
 if (rawObj != nullptr) {
 return rawObj;
 }
 return nullptr;
 }
 void ref()
 {
 m_Mutx.lock();
 count++;
 m_Mutx.unlock();
 }
 void deref()
 {
 m_Mutx.lock();
 count--;
 m_Mutx.unlock();
 if (count == 0)
 {
 delete rawObj;
 count = 0;
 }
 }
};
template <class T>
class SharedPtr
{
 RCObject<T>* m_Rc;
public:
 SharedPtr(T* rawObj)
 {
 m_Rc = new RCObject<T>(rawObj);
 m_Rc->ref();
 }
 ~SharedPtr()
 {
 m_Rc->deref();
 }
 SharedPtr()
 {
 m_Rc = new RCObject<T>();
 }
 SharedPtr(const SharedPtr& rhs)
 {
 m_Rc = rhs.m_Rc;
 m_Rc->ref();
 }
 SharedPtr& operator= (const SharedPtr& rhs) {
 if (this != &rhs) {
 m_Rc->deref();
 m_Rc = rhs.m_Rc;
 m_Rc->ref();
 }
 return *this;
 }
 int getRefCount()
 {
 return m_Rc->getCount();
 }
 T* get() const
 {
 return m_Rc->get();
 }
 T* operator->() {
 return m_Rc->get();
 }
};
#endif

• Incomputable: Could you include information about what the code does? It looks like a shared_ptr. Also, the title should be the name of what you've created (for example, implementation of reference counted smart pointer). How do I ask a good question?
• Diggy.: Try this link. If that one doesn't help, you can try this link. For future reference, do some research before you start posting questions to make sure you aren't posting duplicates, or maybe there is already an answer out there.
• Loki Astari: Note: Shared pointer is much harder than you think it is. Good first try though. But no biscuit this time. But even Scott Myers (who has written several good C++ books) made a mess on his first attempt (according to him in a talk he once gave). Even after good peer review people were finding bugs in his implementation for years (after publication and several fixes).

Answer 1

Safety

The whole point of the smart pointers is that you give up ownership of the pointer; so it (the smart pointer) will delete it if something goes wrong. Unfortunately your smart pointer fails this first test.

SharedPtr(T* rawObj)
{
 m_Rc = new RCObject<T>(rawObj); // if this throws you have
 // leaked your object the one
 // thing a smart pointer should
 // never do.
 m_Rc->ref();
}

If that new throws you leak the pointer that was given to you.

Probably the best way to handle this is the initializer try catch:

SharedPtr(T* rawObj)
try
 : m_Rc(new RCObject<T>(rawObj)) 
{
 m_Rc->ref();
}
catch(...)
{
 delete rawObj;
 throw;
}

Rule of Three

Your RCObject does not obey the rule of three. Now this is a special case as it should only be used by SharedPtr and I can't see a wrong usage. But to protect against future misuse you should make this (RCObject) a private member of SharedPtr.

Though you have the three methods you require for SharedPtr to implement the rule of three you don't actually do it. Su you leak m_Rc in a couple of situations.

{
 SharedPtr ac;
}// Leaked the m_Rc
SharedPtr ac1;
SharedPtr ac2;
ac1 = ac2; // leaked m_Rc from ac1.

RefCount Issue

I don't like setting the Count to zero in RCObject constructors as that requires you to call ref() after creating the object to correctly initialize it.

RCObject() :rawObj(nullptr), count(0){}

The whole point of the constructor is to put the object into a valid state so it can immediately be used.

And here is the bug caused by that misuse.

SharedPtr()
{
 m_Rc = new RCObject<T>();
 // You forgot to increment the ref count here.
 // When this shared pointer is destroyed the count will go to -1
 // not zero.
 // Currently this is not causing an issue because your class is
 // so simple but this breaks the invariant that everybody thinks
 // you have and thus as the class is expanded it will cause an
 // error at some point.
}

Pointless Test

Don't see the point in the test.

T* get()
{
 if (rawObj != nullptr) {
 return rawObj;
 }
 return nullptr;
}

if the pointer is null you will return nullptr. Why not just return the pointer?

Learn to use RAII

void ref()
{
 m_Mutx.lock();
 count++;
 m_Mutx.unlock();
}

Any time you see the pattern:

SetUp
Work
TearDown

You should think RAII and wrap it. It may seem usless; but as your class expands it will ensure that your class stays exception safe. Also there are so many helper classes out there that it is not really that much work.

void ref()
{
 std::lock_guard<std::mutex> lock(m_Mutx);
 count++;
}

Stop using leading underscore in identifiers

RCObject(T* _rawObj):rawObj(_rawObj)
{}

The rules for a leading underscore are non trivial and most people don't know them. So even if you do most other people will get confused and panicky.

The case where you use them it is not required anyway:

// This is perfectly valid.
// Does what you expect and quite normal in C++ code.
RCObject(T* rawObj)
 : rawObj(rawObj)
{}

Other Stuff.

There are a whole bunch of other stuff you should do take a look at these three articles I wrote:

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

• ankit srivastava: Not sure about the below case: { SharedPtr ac; }// Leaked the m_Rc SharedPtr ac1; SharedPtr ac2; ac1 = ac2; // leaked m_Rc from ac1 My assignment operator is doing a derf on current object. SharedPtr& operator= (const SharedPtr& rhs) { if (this != &rhs) { m_Rc->deref(); m_Rc = rhs.m_Rc; m_Rc->ref(); } return *this; }
• Loki Astari: @ankitsrivastav: Your handling of the data object rawObj works. Your handling of the object m_Rc does not. I see two calls to new in SharedPtr but zero calls to delete.
• aggsol: Instead of a mutex have a look at std::atomic for the ref counting
• Arne Vogel: In addition to the issues mentioned by Martin York, the code exhibits a data race if RCObject<T>::defref() is called twice, concurrently (or e.g., deref() on one, and getCount() on another thread), for details, see my related answer on Stack Overflow. Also, aggsol is right about suggesting to use std::atomic. (For starters, I would recommend sticking with the default memory order so as to reduce the likelihood of further data races resulting from misuse of atomics.)
• Loki Astari: @ArneVogel That's a good point. Then your comment is valid. I should have pointed out that he should have split it into two classes.