[Telepathy] tq-qt4 refcount proposal

[Jonathon Jongsma]

Andre Moreira Magalhaes wrote:
> First version of the proposal with all suggestions is up on my shared 
> branch:
> http://git.collabora.co.uk/?p=user/andrunko/telepathy-qt4.git;a=shortlog;h=refs/heads/shared
> 
> BR
> Andre

OK, I took a look at the current implementation, and here are my notes.  It's obviously 
possible that i've misunderstood or misinterpreted something, so feel free to challenge me 
on any of these.

[explicit inline SharedPtr(T *d) : d(d) { if (d) { d->ref(); } }]
boost::shared_ptr uses a templated constructor for this case (e.g.  template
<class Y> SharedPtr(Y *d) ...) which allows it to somehow remember the actual
pointer type passed so that if it was a different type that is convertible to
T*, it will call delete with its original type and thus will not leak even if Y
does not have a virtual destructor). Granted, they probably use some black magic
to accomplish this, but it might be worth considering.  For example, consider
this contrived example:
   class A {};
   class B : public A {
   public:
       B() : x(new int[1000]) {}
       ~B() { delete [] x; }
   private:
       int * x;
   }
   // This will leak 1000*sizeof(int) when it goes out of scope
   SharedPtr<A> shared(new B());
   // whereas this does not leak:
   boost::shared_ptr<A> boost_shared(new B());

[SharedData]
Speaking of virtual destructors, SharedData should probably have one if it's meant to
be the base class of all ref-counted classes. Also, the name is rather

[template<class X>
inline SharedPtr(const SharedPtr<X> &o)]
I don't think this constructor is a good idea.  I assume this is designed so that
you can easily convert from a derived class pointer to a base class pointer as you
would with raw pointers, but I think it's much better to make the programmer state
explicitly what they want to do.  And in any case, dynamic_cast is probably the more
appropriate cast in this situation.  See also
boost::static_pointer_cast/dynamic_pointer_cast/etc or
Glib::RefPtr::cast_static/cast_dynamic/etc

[inline SharedPtr(const WeakPtr<T> &o)]
It's probably best to make this explicit as well so that converting from a weak
pointer to a shared pointer is only done when it's intentional. You also might
consider something like the boost::weak_ptr::lock() API that allows you to
easily create a SharedPtr from a WeakPtr

[Copy constructor and operator=()]
You might consider using the swap() idiom like Glib::RefPtr uses, it's quite
elegant and allows greater code reuse:
http://svn.gnome.org/viewvc/glibmm/trunk/glib/glibmm/refptr.h?view=markup#l228

[~SharedPtr()]
Perhaps ~SharedPtr() should just call reset()?  You'd be doing an extra 'd = 0'
operation during destruction, but you'd eliminate some code duplication.

[data()]
You may want to think about whether data() is really necessary as a public API.
It can certainly be useful at times to access the underlying raw pointer, but it
can also encourage improper use by application developers, since they can easily
grab the pointer out and store it somewhere, which circumvents the
reference-counting

[General]
Be very careful about passing smart pointers as const references since that
circumvents the smart pointer's reference-counting mechanism.  It looks like
everywhere you use const references in the implementation of the smart pointer
are ok, but just keep it in mind (especially when you actually use these classes
other Telepathy API)

You should make your API reference crystal clear that this class is only for use
with classes derived from SharedData.  No matter how obvious you think it is,
people will still assume that it's a generic smart pointer that they can use
with their arbitrary classes (this is from my experience with glibmm)

I don't know if the goal is for this class to be threadsafe or not, but strictly
speaking, it's not. The reference count involves indirection, so although the
the integer operations are atomic, the following code is not atomic as a whole:
'if (d) {d->ref();}'.  So while the reference count itself can not be
corrupted, there are corner cases that could cause problems: (e.g. thread A
could test that d is not null, then another thread could decrement the ref count
to zero and delete d, and then the thread A would attempt to call d->ref()).


-- 
Jonathon Jongsma
Collabora Ltd