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

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