Are these classes well implemented?

[Deimos]

Hey again,

After trying to produce an actual game using my first game engine, I reached the conclusion that it sucked. It's mostly because I didn't pay much attention to memory management when I was developing it, I relied too much on our friends new and delete. As a result, I got some nice segfaults in which I lost too much time fixing. However, I'm happy about what I learnt. :)

Moving on, I decided to start out fresh and before any coding I read a bit on Design Patterns, and I feel that made me think slightly differently about problems and it also made some things quite clearer. :) Now, onto the matter for which I'm here...These are some classes which will be used by my future games. I tried to make them as clean as possible but since all my games will be depending on them I want to make sure that everything is OK. Any suggestion, remark, comment would be greatly appreciated. :)
They are:
An interface for all reference-counted classes;
A smart-pointer;
A singleton interface.

Here's the code:

#ifndef I_REF_COUNTED_H
#define I_REF_COUNTED_H

class IRefCounted
{
    public:
        IRefCounted() : m_RefCount(0) {}
        virtual ~IRefCounted() {}
        void Grab() { IncRef(this); }
        void Release() { DecRef(this); }
    private:
        IRefCounted(const IRefCounted& obj) {};

        static void IncRef(IRefCounted *pObj)
        {
            if(pObj)
            {
                pObj->m_RefCount++;
            }
        }
        static void DecRef(IRefCounted *pObj)
        {
            if(pObj)
            {
                pObj->m_RefCount--;
                if(pObj->m_RefCount <= 0)
                {
                    delete pObj;
                    pObj = NULL;
                }
            }
        }

        int m_RefCount;
};

#endif

#ifndef C_SMART_POINTER_H
#define C_SMART_POINTER_H

template<class T>
class CSmartPointer
{
    public:
        CSmartPointer() : m_pT(NULL) {}
        CSmartPointer(T* pT) : m_pT(pT) { GrabRef(); }
        CSmartPointer(const CSmartPointer& ptr) : m_pT(ptr.m_pT) { GrabRef(); }
        ~CSmartPointer() { ReleaseRef(); }

        CSmartPointer& operator=(T* pT)
        {
            if(IsNull())
            {
                if(pT)
                {
                    //The previous pointer was NULL, so no need for ReleaseRef()
                    //Assign pointer and grab reference
                    m_pT = pT;
                    GrabRef();
                }
            }
            else
            {
                //The previous pointer is no longer in use
                ReleaseRef();
                //Assign new pointer and grab a new Reference
                m_pT = pT;
                //GrabRef is "NULL-safe"
                GrabRef();
            }
            //So as to allow multiple assignments ptr1 = ptr2 = pr3...
            return *this;
        }
        //Only difference is the need to access to CSmartPointer's internal pointer
        CSmartPointer& operator=(const CSmartPointer& ptr)
        {
            if(IsNull())
            {
                if(ptr.pT)
                {
                    m_pT = ptr.pT;
                    GrabRef();
                }
            }
            else
            {
                ReleaseRef();
                m_pT = ptr.pT;
                GrabRef();
            }
        }
        //Checking for NULL and equality
        bool operator!() { return m_pT == NULL; }
        bool operator==(const T* pT) { return pT == m_pT; }
        bool operator==(const CSmartPointer& ptr) { return ptr.m_pT == m_pT; }
        //Simulating a "raw" pointer usage trough operator overloading
        T* operator*(){ return m_pT; }
        T* operator->() { return m_pT; }

    private:
        void GrabRef()
        {
            if(m_pT)
                m_pT->Grab();
        }

        void ReleaseRef()
        {
            if(m_pT)
                m_pT->Release();
        }

        bool IsNull() { return m_pT == NULL; }

        T* m_pT;
};

#endif

#ifndef I_SINGLETON_H
#define I_SINGLETON_H

#include "IRefCounted.h"
#include "CSmartPointer.h"

template<class T>
class ISingleton : public IRefCounted
{
    public:
        virtual ~ISingleton() {}
        static T* GetInstance() { if(!m_pInstance) m_pInstance = new T; return *m_pInstance; }
    protected:
        ISingleton() {}
    private:
        static CSmartPointer<T> m_pInstance;
};

template<class T>
CSmartPointer<T> ISingleton<T>::m_pInstance = NULL;

#endif

Sorry about pasting all this stuff here. :)

Thanks in advance and Best Regards,
Deimos

[helios]

I think there's a pretty good chance that line 29 in your first snippet will cause a segfault. When you enter that function from Release() there's a chance you'll end up deleting this.

http://www.cplusplus.com/reference/std/memory/auto_ptr/
Also, http://www.hpl.hp.com/personal/Hans_Boehm/gc/

My OO may be a little rusty, but how are ISingleton and IRefCounted supposed to be interfaces when none of their functions are purely virtual?

Personally, I think C/++ goes with garbage collection as much as Java/C# goes with manual memory management. If you want to avoid it, I think it'd be best to just use automatic storage duration on the stack. But hey, whatever rocks your boat.

[PanGalactic]

Any reason you aren't just using a Boost shared pointer for your ref counting needs?