Before you say anything: Due to project constraints, we cannot use Boost and we cannot use C++11 (yet; perhaps this will change some day).

The fact that I was unable to use smart pointers was nagging on me for some time, so I decided to cough up my own implementation, and it was surprisingly simple. So, I'd like you to have a look at it. Btw. I decided to use a linked list instead of a ref-counter to avoid newing counters.

I ran some unit tests and couldn't find a problem. Perhaps you can suggest improvements?


Here is my SharedPtr.h:

#ifndef __NET_UTIL_SHARED_PTR_H__
#define __NET_UTIL_SHARED_PTR_H__

#include <stddef.h>

// since we cannot use boost nor C++11, nor TR1, we have to reinvent the wheel, sigh
namespace net {
  namespace util {
    class SharedPtrBase {
      private:
        mutable SharedPtrBase const* left;
        mutable SharedPtrBase const* right;
      protected:
        SharedPtrBase();
        SharedPtrBase(SharedPtrBase const& from);
        ~SharedPtrBase();
        void leave() const; // Note: leaving is idempotent
        void join(SharedPtrBase const* where) const;
        void assertWrapper(bool) const;
      public:
        bool isSingleton() const;
    };

    template <typename T> struct Deallocator {
      private:
        bool doDelete; // not const to be def. assignable
      public:
        // Implicit Constructor on purpose!
        Deallocator(bool doDelete = true) : doDelete(doDelete) {}
        bool willDelete() const {
          return doDelete;
        }
        void operator()(T* t) const {
          if (doDelete)
            delete t;
        }
    };

    template <typename T, typename Delete = Deallocator<T> >
    class SharedPtr : private SharedPtrBase {
      private:
        Delete del;
        T* ptr;
        void drop() {
          if (ptr && isSingleton()) {
            del(ptr);
            ptr = NULL;
          }
          leave();
        }
      public:
        // SharedPtr(p,false) will not delete the pointer! Useful for Stackobjects!
        explicit SharedPtr(T* ptr = NULL, Delete del = Delete())
          : SharedPtrBase(), del(del), ptr(ptr) {
        }
        SharedPtr(SharedPtr const& from)
          : SharedPtrBase(from), del(from.del), ptr(from.ptr) {
        }
        ~SharedPtr() {
          drop();
        }
        SharedPtr& operator=(SharedPtr const& from) {
          if (&from != this) {
            drop();
            del = from.del;
            ptr = from.ptr;
            join(&from);
          }
          return *this;
        }
        bool operator==(SharedPtr const& with) const {
          return ptr == with.ptr;
        }
        bool operator==(T* with) const {
          return ptr == with;
        }
        bool operator<(SharedPtr const& with) const {
          return ptr < with.ptr;
        }
        bool operator<(T* with) const {
          return ptr < with;
        }
        T& operator*() const {
          return *operator->();
        }
        T* operator->() const {
          assertWrapper(ptr);
          return ptr;
        }
        //T* release() {
        //  leave();
        //  T* const p = ptr;
        //  ptr = NULL;
        //  return p;
        //}
    };
  }
}

#endif

And my SharedPtr.cpp:

#include "util/SharedPtr.h"

#include <assert.h>

namespace net {
  namespace util {
    SharedPtrBase::SharedPtrBase()
      : left(this), right(this) {
    }
    SharedPtrBase::SharedPtrBase(SharedPtrBase const& from)
      : left(this), right(this) {
      join(&from);
    }
    SharedPtrBase::~SharedPtrBase() {
      leave();
    }
    // Note: leaving is idempotent.
    // Try as much as you like, you will never leave yourself.
    void SharedPtrBase::leave() const {
      left->right = right;
      right->left = left;
    }
    void SharedPtrBase::join(SharedPtrBase const* where) const {
      assert(where);
      leave();
      assert(where->left && where->right);
      left = where->left;
      left->right = this;
      right = where;
      right->left = this;
    }
    void SharedPtrBase::assertWrapper(bool condition) const {
      assert(condition);
    }
    bool SharedPtrBase::isSingleton() const {
      return left == this;
    }
  }
}
  • There should be reinvent-the-wheel tag! – bitmask Nov 26 '11 at 23:05
  • 2
    PS. Don't use double underscore in your identifiers. Or identifiers that begin with single underscore followed by a capitol letter. Here __NET_UTIL_SHARED_PTR_H__ is reserved for the implementation. – Martin York Nov 27 '11 at 2:23
up vote 4 down vote accepted

OK. Lets see..

void SharedPtrBase::leave() const {
  left->right = right;
  right->left = left;
}

This removes you from the list, but does not reset your own left and right pointers. This is OK when you call it from the destructor() or join(). But what about release()?

Lets look at release().

    T* release() {
      leave();
      T* const p = ptr;
      ptr = NULL;
      return p;
    }

You have just released the pointer so that other functions can use it (and potentially free it). But all other members in the ring still have it (the pointer). What you should do is set the pointer to NULL in all members in the ring.

I just spent two minutes and found two bugs. Implementing Shared pointer is not as simple as you think. It took them a while to get the boost version working correctly in all situations. So though it is a nice project implementing it safely is not as simple as you think.

SharedPtrBase base is used to implement the underlying ring. But users can now potentially use it as a base class pointer:

SharedPtrBase*  x = new SharedPtr<int>();

delete x;  // Undefined behavior (destructor is not virtual).

Yes I know this is stupid. But the point is people get into weird situations and it may happen. It is your responsibility to prevent your class from being used incorrectly. A couple of solutions, make it privately inherited (as you have done), or the method I would use is to make a member (rather than a parent) to control the ring. Then your class can not be abused (and you do not need to make the destructor virtual).

I personally (but this one is a personal opinion) don't like the use of a pointer in join:

void join(SharedPtrBase const* where) const;

By making it a reference you don't need to check that where is NULL and it should always be used with a known object on the other end (where). I would just have made this a reference as that implicitly conveys this requirement to the user and thus reduces the chances of misuse.

  • Thanks for your feedback, but your last comment is not valid, because I use private inheritance. Regarding release this is the way release was supposed to work. Actually I think I will remove the method completely, because it is not needed right now, and as you point out it has unclear semantics (for me it says, "THIS object relinquishes control", buy you understand it as "EVERY object relinquishes control"). Besides that, any objections? – bitmask Nov 27 '11 at 0:13
  • 2
    You are correct I missed the private inheritance. But personally I would still make it a member. – Martin York Nov 27 '11 at 0:16
  • Regarding "smart pointers are not trivial": Absolutely, and I truly hate reinventing the wheel, but the difference is, I do not make the claim that my implementation comes anywhere near the boost implementation (I'd never be that delusional). However, as long as I need less functionality the feature-subset might be small enough for me to manage, right? And the choice is: My own smart pointers or no smart pointers at all! – bitmask Nov 27 '11 at 0:20
  • 1
    <quote>"THIS object relinquishes control"</quote> The trouble is that THIS is really the collection of shared pointers. What you may have been trying to implement is reset() which resets a single shared pointer to NULL (or another pointer) but if you are releasing the pointer then all members of the group must release it otherwise there may be multiple points of ownership and the point is you want to have a single point of ownership (in this case group ownership but a single point of ownership). – Martin York Nov 27 '11 at 0:28
  • Yes, I'm convinced. Doesn't make sense in my case, ... too expensive, ... removed. :) – bitmask Nov 27 '11 at 0:32

Your Answer

 
discard

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.