Thread: A Generic Reference-Counted Pointer Class (4 Bubba)

>> If I must say something, I'd say make ref_count() const as well.
Good catch.

>> Why not? Just assign the deleter along with the pointer.
Yes, I know - I gave a bad excuse for that one.
New excuse: Doing this requires moving the functor type out of the template parameter list for ref_ptr<>. In order to do that, and still support a templated functor type, we'd have to mimic the boost::shared_ptr implementation (which is similiar to the functor implementation used by my thread class). I just didn't want the implementation to get that big for this "tutorial design review of a smart pointer" thread

>> For getting both of these, you either have to [keep what you have] or use something akin to Boost.Function.
I still don't see the benefit of using boost::function[N] or something like it. When the "ZeroRef" functor is a templated type, both of your example functors will always work (unless your compiler is broken). Notice that boost::shared_ptr simply uses a template argument for the functor type as ref_ptr does (not to say that they do it in the same way).

>> Also, perhaps you could add a second 'built-in' delete functor, delete_array
Sounds good to me. Copy and rename the delete_ptr implementation and add "[]" in the right spot.

>> I also wrote a [] operator, and scattered liberal asserts around
Nice. Again, my philosophy is not to go out of my way to prevent the user from shooting themselves in the foot. I'll usually go as far as debug-only runtime assertions to let users know where the gun pointed

>> This will break on self-assignment, won't it?
Did I mention that I'm smoke'n and drink'n?
Third time's a charm (I hope):

Code:

    // copy 'rcp' into '*this' and increment reference count
    void copy(const ref_ptr &rcp)
    {
        m_refs = rcp.m_refs;
        m_pointer = rcp.m_pointer;
        (*m_refs)++;
    }//copy

    // decrement reference count and call m_unload on 0
    void dec_ref()
    {
        if (--(*m_refs) == 0)
        {
            delete m_refs;
            m_unload(m_pointer);
        }//if
    }//dec_ref
…
    ref_ptr& operator=(const ref_ptr &rcp) 
    {
        if (m_refs != rcp.m_refs)
        {
            dec_ref();             
            copy(rcp);
        }//if

        return *this;
    }//operator=

    size_t ref_count() const {return *m_refs;}

In the name of simplicity, m_refs is assumed to be allocated always.

>> Otherwise you'll have reference counting for NULL
Choosing the more simplistic approach, this is what ref_ptr currently does. It could be improved without adding much more to the class if the call to new in the constructor is removed (better exception safety) and if m_refs is allocated only when referencing non-null pointers - although it would take me 3 or more iterations to get it right.

I think it's good for readers that we've identified alot of the weakness', bugs, and foot-shootings that exist in ref_ptr, as well as what boost::shared_ptr has to offer in terms safety, efficiency, and ease of use. Keep it come'n!

gg

>>It could be improved without adding much more to the class if the call to new in the constructor is removed (better exception safety) and if m_refs is allocated only when referencing non-null pointers
You were right All it took was replacing the assert's in my code with if(refCount != NULL).
Here's my updated code:

Code:

#include <cassert>

template <class T>
class RCSmartPtr
{
public:
	typedef void (*ZeroRefDeleteProc_t) (T* ptr);
	static void genericDelete(T* ptr) {delete ptr;}
	static void arrayDelete(T* ptr) {delete[] ptr;}

public:
	explicit RCSmartPtr(T* ptr, ZeroRefDeleteProc_t delProc = &genericDelete) :
		refCount(NULL), ptr(ptr), zrDeleteProc(delProc)
	{
		if(ptr != NULL)
			refCount = new unsigned long(1);
	}

	RCSmartPtr(const RCSmartPtr& orig) :refCount(orig.refCount), ptr(orig.ptr), zrDeleteProc(orig.zrDeleteProc)
	{
		if(refCount != NULL)
			++(*refCount);
	}

	~RCSmartPtr()
	{ closeRef(); }

	RCSmartPtr& operator=(const RCSmartPtr& orig)
	{
		if(orig.refCount == refCount)	//If assigning to itself, quit
			return *this;

		closeRef();
		
		ptr = orig.ptr;
		zrDeleteProc = orig.zrDeleteProc;
		refCount = orig.refCount;
		if(refCount != NULL)
			++(*refCount);

		return *this;
	}

	T* operator->() const   {assert(ptr != NULL); return ptr;}
	T& operator*() const   {assert(ptr != NULL); return *ptr;}
	T& operator[](unsigned long index) const   {assert(ptr != NULL); return ptr[index];}

	T* get() const   {return ptr;}
	operator bool() const   {return (refCount != NULL);}

protected:
	void closeRef()
	{
		if(refCount != NULL)
		{
			if(--(*refCount) == 0)
			{
				delete refCount;
				zrDeleteProc(ptr);
				ptr = NULL;
			}
			refCount = NULL;
		}
	}

	ZeroRefDeleteProc_t zrDeleteProc;
	unsigned long* refCount;
	T* ptr;
};

And a random, garbled mess of assignment/construction/destruction to go with it:

Code:

#include <windows.h>

struct X
{
	~X() {MessageBox(NULL, "FJDSLKFJ", "", MB_OK);}
	int x, y;
};

int main()
{
	MessageBox(NULL, "Create", "",MB_OK);
	RCSmartPtr<X> pX(new X[5], &RCSmartPtr<X>::arrayDelete);
	pX->x = 5;
	pX->y = 10;
	const RCSmartPtr<X> x(pX);
	const RCSmartPtr<X> y = x;
	RCSmartPtr<X> z(NULL);
	const RCSmartPtr<X> n = z;
	y.~y();
	n.~n();
	n.~n();
	RCSmartPtr<X> m = n;

	pX[2].x = 1;
	pX[2].y = 3;
	std::cout << pX[2].x << pX[2].y << std::endl;
	std::cout << pX->x << " " << pX->y << std::endl;
	std::vector< RCSmartPtr<X> > vect;
	vect.push_back(pX);
	vect.push_back(pX);
	vect.push_back(pX);
	vect.erase(vect.begin() + 2);
	vect.push_back(pX);
	vect.push_back(pX);
	pX = pX;
	vect.push_back(pX);
	MessageBox(NULL, "Create", "",MB_OK);
	RCSmartPtr<X> pY(new X());
	pY = pX;
	pX.~pX();
	pX.~pX();
	pX = RCSmartPtr<X>(NULL);
	x.~x();
	pY.~pY();
	MessageBox(NULL, "Begin clear", "", MB_OK);
	vect.clear();

	MessageBox(NULL, "k", "k", MB_OK);
	RCSmartPtr<X> k(new X());
	k = RCSmartPtr<X>(new X());
	k = RCSmartPtr<X>(NULL);
	return 0;
}

Messageboxes should appear in this order:
-Create
-Create
-DSFLKSDJF (destroy)
-Begin Clear
-DFKLSJDLSKF (x5)
-k
-DFDSKLJFSD (x2)

This one allows for NULL pointers without reference counting, and protects against the invalid pointer dereferencing caused by explicit destructor calls (even though you'd really have to be trying if you decided to do that).

>>if the call to new in the constructor is removed (better exception safety)
I have never in my life come across an exception caused by new, so at the moment this is of little concern to me. Anyway, how does it matter where an exception occurs? The object is supposed to get destroyed properly regardless. Unless you're referring to what will happen in the destructor if the members aren't initialized yet?

The one I came up with earlier. As a warning, I meant to test it tonight, but did not get a real chance to. (It implements the functor design.)

Code:

#ifndef REF_COUNTED_PTR_HH
#define REF_COUNTED_PTR_HH

#include "generic_delete.hh"

template<class T, class ZeroRefT = generic_delete<T> >
class ref_counted_ptr
{
public:

   explicit ref_counted_ptr(T* ptr);
   ref_counted_ptr(const ref_counted_ptr& src);
   
   ~ref_counted_ptr();
   
   ref_counted_ptr& operator=(const ref_counted_ptr& src);
   
   operator bool() const;

   T& operator*() const;
   
   T* operator->() const;
   
   size_t ref_count() const;
   
private:
   void clear();
   void copy(const ref_counted_ptr& src);
   
private:
   T* m_ptr;
   size_t* m_ref_ct;
};

template<class T, class ZeroRefT>
ref_counted_ptr<T, ZeroRefT>::ref_counted_ptr(T* ptr)
   : m_ptr(ptr), m_ref_ct(0)
{
   if(m_ptr)
   {
      m_ref_ct = new size_t;
      *m_ref_ct = 1;
   }
}

template<class T, class ZeroRefT>
ref_counted_ptr<T, ZeroRefT>::ref_counted_ptr(const ref_counted_ptr& src)
{
   copy(src);
}

template<class T, class ZeroRefT>
ref_counted_ptr<T, ZeroRefT>::~ref_counted_ptr()
{
   clear();
}

template<class T, class ZeroRefT>
ref_counted_ptr<T, ZeroRefT>& ref_counted_ptr<T, ZeroRefT>::operator=(const ref_counted_ptr& src)
{
   if(this == &src || m_ptr == src.m_ptr)
   {
      return *this;
   }
   clear();
   copy(src);
   return *this;
}

template<class T, class ZeroRefT>
ref_counted_ptr<T, ZeroRefT>::operator bool() const
{
   return (m_ptr != 0);
}

template<class T, class ZeroRefT>
T& ref_counted_ptr<T, ZeroRefT>::operator*() const
{
   if(!m_ptr)
   {
      throw -1;
   }
   return *m_ptr;
}

template<class T, class ZeroRefT>
T* ref_counted_ptr<T, ZeroRefT>::operator->() const
{
   return m_ptr;
}

template<class T, class ZeroRefT>
size_t ref_counted_ptr<T, ZeroRefT>::ref_count() const
{
   if(!m_ref_ct)
   {
      return 0;
   }
   return *m_ref_ct;
}

template<class T, class ZeroRefT>
void ref_counted_ptr<T, ZeroRefT>::clear()
{
   if(!m_ref_ct)
   {
      return;
   }
   if(--*m_ref_ct == 0)
   {
      ZeroRefT del;
      del(m_ptr);
      delete m_ref_ct;
      m_ref_ct = 0;
   }
}

template<class T, class ZeroRefT>
void ref_counted_ptr<T, ZeroRefT>::copy(const ref_counted_ptr& src)
{
   m_ptr = src.m_ptr;
   m_ref_ct = src.m_ref_ct;
   if(m_ref_ct)
   {
      ++*m_ref_ct;
   }
}

#endif

And the generic delete:

Code:

#ifndef GENERIC_DELETE_HH
#define GENERIC_DELETE_HH

template<class T>
struct generic_delete
{
   void operator()(T*& ptr)
   {
      delete ptr;
      ptr = 0;
   }
};

#endif

Note, you will see 'throw -1' in the ref_counted_ptr code. There is, in fact, a very good reason for this, namely the following: I did not have the time to search for an appropriate standard exception, or if necessary, derive my own from std::exception... So yeah, it's really just a placeholder.

>> Why not? Just assign the deleter along with the pointer.
Yes, I know - I gave a bad excuse for that one.
New excuse: Doing this requires moving the functor type out of the template parameter list for ref_ptr<>. In order to do that, and still support a templated functor type, we'd have to mimic the boost::shared_ptr implementation (which is similiar to the functor implementation used by my thread class). I just didn't want the implementation to get that big for this "tutorial design review of a smart pointer" thread

I fail to see the problem. You don't need to implement anything, that's what Boost.Function does for you.

Code:

template <typename T>
class happy_ptr
{
  boost::function<void (T*)> deleter;
  T * pointer;
  long * count;

public:
  template <typename D>
  happy_ptr(T *p, D del = default_deleter<T>())
    : pointer(p), deleter(del), count(new long(0)) // Exception-unsafe
  {}

  ~happy_ptr() {
    if(--(*count) == 0) {
      deleter(p);
      p = 0;
      delete count;
      count = 0;
    }
  }
};

Everything else stays as it is.

>> For getting both of these, you either have to [keep what you have] or use something akin to Boost.Function.
I still don't see the benefit of using boost::function[N] or something like it. When the "ZeroRef" functor is a templated type, both of your example functors will always work (unless your compiler is broken). Notice that boost::shared_ptr simply uses a template argument for the functor type as ref_ptr does (not to say that they do it in the same way).

The benefit is that you don't need the additional template parameter and thus can pass around the pointers more freely. For example, you could have a std::list that stores smart pointers to integers from various sources. They could come from malloc, new, get_temporary_buffer, GlobalAlloc, LocalAlloc, HeapAlloc... and the deletion semantics of each one would still be ensured. If the deleter is a template parameter, this is not possible, because different deleters cause the pointer to be of a different type.

Code:

template<class T> class shared_ptr
{

shared_ptr doesn't have a template parameter for the deleter. shared_ptr emulates Boost.Function in a very simple way, in the header boost/detail/shared_count.hpp, using the classes shared_count (hides the complexity from the smart pointer and makes it exception-safe), sp_counted_base (stores the refcount, has a few virtuals) and sp_counted_base_impl (templated on the deleter type, stores it and makes it callable via the virtuals).
The untemplated_base + templated_derived pattern is the same in shared_ptr as in any, function and my own any_iterator.

Supporting broken compilers is always very low on my priority list.

The polymorphic base class with templated derivations is not a workaround for missing member templates, but a workaround for storing various specializations of the same template in a single location. Look at the implementation of Boost.Any, it uses the same concept. Boost went with the polymorphic solution to store various kinds of function objects. Using a Boost.Function instead would not have helped, because Boost.Function does the very same thing internally. My own usage is just to illustrate that it is possible to do it that way without too much work.

Where do you get the idea that a vtable size plays any role in object size? In the most common ABI (i.e. that of gcc and MSVC), as long as we only have single non-virtual inheritance, each object with a vtable requires exactly 4 bytes of additional space per object, to hold the vptr.

Perhaps it can be used as a workaround (I don't see how, really), but it isn't in Boost's smart pointer. There, there is still a member template constructor. shared_ptr has a lot of member templates. And it compiles fine in VC++6, if I remember correctly, it's 5 that doesn't support member templates at all.

Also, what it does cannot be emulated with member templates or partial specialization anyway.

>> ...but it isn't in Boost's smart pointer.
Yes it is. Have a look at <boost\detail\shared_count.hpp>

>> And it compiles fine in VC++6
Correct.

>> but it does have a member template constructor - which needs the workaround for MSVC.
Well, I based that statement on attempts to use boost::function1<void, T*> but kept getting "internal compiler errors".
The fact that it's a member template constructor probably isn't the reason why 6.0 had problems. Like you said, shared_ptr has plenty of template member constructors. I'll work on it some more to see if I can get it to work under 6.0.
....
<working on solution>
....
I got something that works under 6.0. Unfortunately, you have to remove the default value from the constructor's second parameter.
Using ref_ptr as a base, here are the changes:

Code:

template <class T>
class ref_ptr
{
protected:
    // boost::function syntax for broken compilers (see boost docs)
    boost::function1<void, T*> m_unload; 

    // don't forget to copy m_unload as well within copy()

public:
    template <class D>
    explicit ref_ptr(T *p, const D &f) 
        : m_pointer(p), m_unload(f), m_refs(new size_t(1)) {}
};//ref_ptr

gg

Rushing posts at work can prove necessary.


Well, it doesn't really matter. Having different implementations for one problem is part of the fun.

good work guys. I'll be using this template quite a lot. don't you think you should inline the pointer accessors, though? and you might repost the entire template for posterity.

Watch out, Sebastiani. As Codeplug said somewhere, this is more a "let's see what the issues are and how it can be implemented" code, sparking some quite interesting discussion.

For production use, I'd recommend you use the tested and safe smart pointer classes from the Boost libraries.

other than the fact that the counter is dynamically allocated, making it exception/thread unsafe, I don't see any problems. I think I'll give it a go too...