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?

migrated from programmers.stackexchange.com Mar 5 '14 at 1:16

This question came from our site for professionals, academics, and students working within the systems development life cycle.

  • 11
    C++ includes smart pointers already, why implement another? Also, this question is probably better suited to the Code Reviews Stack Exchange. – user31517 Mar 4 '14 at 20:10
  • @John Gaughan: I can think of several valid reasons, though they all contain one of the words "training", "study", "drilling" or the like. – phresnel Mar 5 '14 at 7:56
  • 2
    @Raffa50: I think the first lesson you should learn this time is to not introduce new "keywords" using #define (i.e., if you example compiles for you, then only because you did #define destructor). #define-Macros are frowned upon in 99.9% of cases for very good reasons. – phresnel Mar 5 '14 at 7:58
  • @phresnel I agree, but the author was not clear on the reason for doing this. That is why I pointed out that C++ already has smart pointers, and asked "why implement another?" This may be a learning exercise, or it may be reinventing the wheel. One is fine, one is a bad idea. – user31517 Mar 5 '14 at 14:59
  • Well because this smart pointer have is and as that works like C#... can I do the same with std::share_ptr? – Raffa50 Mar 5 '14 at 17:31

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.

  • Ref(int ptr) allows me to do: return NULL; – Raffa50 Mar 5 '14 at 18:14
  • 1
    @Raffa50, it also allows you invalid examples (return 25; for example). If you want to simply return NULL, define Ref::Ref(nullptr_t); Then, use return nullptr; in client code. – utnapistim Mar 6 '14 at 9:10

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.

  • Ref(int ptr) allows me to do: return NULL; – Raffa50 Mar 5 '14 at 18:14

Your Answer

By clicking "Post Your Answer", you acknowledge that you have read our updated terms of service, privacy policy and cookie policy, and that your continued use of the website is subject to these policies.

Not the answer you're looking for? Browse other questions tagged or ask your own question.