shared_ptr code implementation

I have never used std::shared_ptr before and I don't really know how this smart pointer actually works but I decided to create one to verify my knowledge. Here is my code :

namespace myStd
{
    template
    class shared_ptr
    {
        public : 

        shared_ptr() : refCount(nullptr), t(nullptr) {}
        shared_ptr(T *p) : refCount(new int(1)), t(p) {}

        ~shared_ptr()
        {
            this->destroy();
        }

        shared_ptr(const shared_ptr &p) : refCount(nullptr), t(nullptr)
        {
            if(p.isvalid())
            {
                refCount = p.refCount;
                t = p.t;

                if(isvalid())
                (*this->refCount)++;
            }
        }

        void destroy()
        {
            if(isvalid())
            {
                --(*refCount);
                if((*refCount) destroy();
                    this->refCount = p.refCount;
                    this->t = p.t;

                    if(isvalid())
                    (*this->refCount)++;

                    return (*this);
                }

                if(!isvalid())
                {
                    this->refCount = p.refCount;
                    this->t = p.t;

                    if(isvalid())
                    (*this->refCount)++;

                    return (*this);
                }
                else
                {
                    this->destroy();
                    this->refCount = p.refCount;
                    this->t = p.t;

                    if(isvalid())
                    (*this->refCount)++;                    
                }
            }
            return (*this);
        }

        bool isvalid() const {return (t != nullptr && refCount != nullptr);}
        int getCount() const {if(refCount != nullptr) return (*refCount); else return 0;}

        T* operator ->() {return t;}
        T& operator *() {return *t;}

        private : 
        int *refCount;
        T *t;
    };
}

What am I m

What am I missing?

Several things.

• constructor that takes object of type std::nullptr_t

• constructor that takes object of type shared_ptr where U is derived from T

• boolean conversion operator.

• swap operator

• move semantics

Sure there is more. In Smart pointers you always forget something.


Did I implement this smart pointer right?

YOu implemented the rule of three. So barring any bugs it should be good.


Your suggestions would be appreciated.

I go into a lot of detail in my articles:

Unique Pointer

Smart Pointer

Constructors

Don't use this.

It is not very normal to see this-> in C++ code (unlike Java).

I actually consider the use of this-> as bad practice. You only need to use this->member if there is some ambiguity between local/parameters and member names.

If you have some ambiguity then you have done a bad job at naming and you are using this-> to cover up the problem. Prefer to fix the problem as some poor maintainer is going to come along later and use the wrong variable because of your laziness.

I normally turn on the compiler warning that tells my if a local variable is shadowing another thus forcing me to have unique (and meaningful names).

One Line One Statement

shared_ptr(const shared_ptr &p) : refCount(nullptr), t(nullptr)

To me this looks exactly like multiple assignments packed onto the same line.

varOne = 5; varTwo = 6; theOtherOne = doStuff(5);

I think it is well accepted that one line should have one statement. Why not apply this to your parameter list. It will make reading the code much easier and also validating order of initialization problems.

shared_ptr(const shared_ptr &p)
    : refCount(nullptr)
    , t(nullptr)
{
    /* Stuff */
}

Construct with a null ptr

shared_ptr(T *p) : refCount(new int(1)), t(p) {}

What happens if you I build with a nullptr?

X*            ptr = nullptr;
shared_ptr data1(ptr);
shared_ptr data2(nullptr); // should have its own constructor.

Double Check of isvalid() is wastefull

If p.isvalid() is true then isvalid() will be true after the copy.

shared_ptr(const shared_ptr &p) : refCount(nullptr), t(nullptr)
    {
        if(p.isvalid())
        {
            refCount = p.refCount;
            t = p.t;

            if(isvalid())
            (*this->refCount)++;
        }
    }

Also this could be much simplified.

shared_ptr(const shared_ptr &p)
        : refCount(p.refCount)
        , t(p.t)
    {
        if(isvalid()) {
            (*refCount)++;
        }
    }

Prefer Copy/Swap Idiom for assignment.

shared_ptr& operator =(const shared_ptr &p)
    {
        if(this != &p)
        {
            /* This LOOKS LIKE A BUG */
            /* Don't think the ! should be there. */
            if(!p.isvalid())
            {
                this->destroy();
                this->refCount = p.refCount;
                this->t = p.t;

                if(isvalid())
                (*this->refCount)++;

                return (*this);
            }

            if(!isvalid())
            {
                this->refCount = p.refCount;
                this->t = p.t;

                if(isvalid())
                (*this->refCount)++;

                return (*this);
            }
            else
            {
                this->destroy();
                this->refCount = p.refCount;
                this->t = p.t;

                if(isvalid())
                (*this->refCount)++;                    
            }
        }
        return (*this);
    }

Sorry that function is big and clumsy and hard to read. It also does not provide the strong exception guarantee. You modify the content of this object doing exception dangerous work (call destroy) before updating the content of this object.

To do any work in exception safe way you need to follow a three step processes.

• Make a copy into temp. -> Dangerious as exceptions can fly.

• Swap the temp and current. -> Safe as swap should not throw exceptions.

• Destroy then temp. -> Safe as the state of current is now good.

This 3 step process is ipitimized by the copy and swap idiom.

shared_ptr& operator =(const shared_ptr &p)
 {
     shared_ptr  temp(p);    // Constructor copies. Destructor destoryes.
     temp.swap(*this);       // Perform an exception safe transfer of state.
     return *this;
 }

Lots of people take this one step further. And do the copy in the parameter:

shared_ptr& operator =(shared_ptr p)  // Notice the pass by value.
 {                                     // this gets your copy.
     p.swap(*this);
     return *this;
 }

Const correctness

These two functions do not mutate the state of the object.

T* operator ->() {return t;}
    T& operator *() {return *t;}

So they should probably be marked as const.

StackExchange Code Review Q#140693, answer score: 7