Smart pointer implementation

Question

I've decided to implement a simple smart pointer:

#pragma once
//simple and basic reference by Aldrigo Raffaele
#include "Common.h"
#include "Exceptions.h"

class MemWrp {
        private:
        uint numref;
        public:
        void* content;

        MemWrp(void* content) {
                this->content = content;
                numref = 1;
        }

        void AddRef() { numref++; }
        uint RemoveRef() { return --numref; }

        destructor MemWrp() { delete content; }
};

template< class T >
class Ref {
        private:
        MemWrp* wrp = NULL;
        public:
        Ref() {}
        Ref(T* ptr) {
                wrp = NULL;
                if (ptr != NULL) wrp = new MemWrp(ptr);
        }
        Ref(const Ref<T>& r) {
                wrp = r.wrp;
                if (wrp != NULL) wrp->AddRef();
        }
        Ref(MemWrp* w) {
                wrp = w;
                wrp->AddRef();
        }

        T* Get() const {
                if (wrp == NULL) return NULL;
                return (T*)wrp->content;
        }

        template<class F>
        bool is() { return dynamic_cast<F*>(Get()) != NULL; }

        template<class F>
        Ref<F> as() {
                if (!this->is<F>()) return Ref<F>::Ref();
                return Ref<F>::Ref(this->wrp);
        }

        Ref<T>& operator=(Ref<T>& rhs) {
                if (this == &rhs) return *this;
                if (rhs.wrp == NULL && wrp != NULL && wrp->RemoveRef() == 0) delete wrp;
                wrp = rhs.wrp;
                if (wrp != NULL) wrp->AddRef();
                return *this;
        }

        Ref<T>& operator=(T* rhs) {
                if (Get() == rhs) return *this;
                if (rhs == NULL && wrp != NULL && wrp->RemoveRef() == 0) delete wrp;
                else if (rhs != NULL) wrp = new MemWrp(rhs);
                return *this;
        }

        bool operator==(const Ref<T>& other) const {
                return (this->Get() == other.Get());
        }

        bool operator==(const int other) const {
                return (this->Get() == (void*)other);
        }

        bool operator!=(const Ref<T>& other) const {
                return !(*this == other);
        }

        bool operator!=(const int other) const {
                return !(*this == other);
        }

        T* operator->() {
                if (this->Get() == NULL) throw NullPointerException();
                return this->Get();
        }
        const T* operator->() const {
                if (this->Get() == NULL) throw NullPointerException();
                return this->Get();
        }

        destructor Ref() {
                if (wrp != NULL && wrp->RemoveRef() == 0) delete wrp;
        }
};

What do you think? How can I subdivide it into .h and .cpp?

Answer 1

What do you think?

For learning RAII/resource management, it's a good idea to write your own smart pointer class. For production code, use std::shared_ptr, std::unique_ptr, or boost smart pointers (if you have to).

Here are some notes on the code itself:

• The code only compiles if you add:

Code:

#define destructor ~

Don't do that! It simply increases the WTFPLOC ("what the fuck!"s per lines of code) ratio of your code and even if it seems like a nice idea now, you will (most probably) not consider it a good idea one year from now, looking at the code.

• MemWrp should:

  • store the data in a strongly-typed pointer (that means, make it a template by pointer type); Otherwise, when you delete it will not invoke the right destructor type unless you use a cast.

  • be a private part of the Ref class; It's an implementation detail; you should not need to create instances of it outside of Ref implementation, and you should not pollute the global namespace with the class name.

  • Use a better name; While using shortened words for names (i.e. MemWrp instead of MemoryWrapped/MemoryWrapper/MemoryWrap etc), this is only a good idea if the shortened words are traditionally used for replacement (e.g. "ptr" for "pointer" is fine, "Mem" for "memory" is fine, "Wrp" can be expanded into about five words easily and leads to confusion).

  • protect it's managed resource (why is MemWrp::content public?)

• Ref class should:

  • be renamed to _Something_Ptr or Ptr_Something_ ("Ref" traditionally represents a reference which has/should have reference semantics). See std::ref and std::cref for comparison. Examples: Shared_Ptr, CountedPtr, ManagedPtr, SmartPtr, etc. Just don't call it Ref.

  • Not allow construction from a pointer converted to int.

The interface of the class suggests that this code is perfectly legal:

class Qxb; // implementation is irrelevant
Ref<Qxb> ref(101); // good luck dereferencing this pointer

• Ref class should (continued):

  • Not need C-style (or any other style) casts; They are not only unnecessary, they are a symptom of bad implementation or design.

  • Disallow construction of a Ref instance from a MemWrp instance, especially since the MemWrp class is not strongly-typed on the stored content.

  • not implement equality comparison with ints. Your class doesn't store ints, it should not have a value equal to them. This will (probably) lead to problems in client code.

Answer 2

Is destructor a new C++ keyword?

In the assignment operator you want to decrement the reference count even when rhs is null.

Calling delete on a void* content in MemWrp won't call the T destructor.

Allowing Ref(int ptr) construction doesn't make much sense, when you later cast ptr to void*.

Is MemWrp* wrp = NULL; legal C++ syntax? I thought you needed to initialize in the constructor.

Returning a default-constructed empty Ref in the as method is counter-intuitive.

You should prefer to use one of the standard smart pointer classes.