[Telepathy] tq-qt4 refcount proposal

[Andre Moreira Magalhaes]

Jonathon Jongsma wrote:
> 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());
>
Done

> [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
>
Done

> [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
>
Done

> [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
>
Done

> [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 
>
>
Done

> [~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.
>
Done, but the other way, reset calls the destructor using swap

> [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
>
This is needed, as many Qt methods as QObject::connect needs the object 
directly, so better doing this than using operator->

> [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)
>
Yep, when docs are added to these classes I will add a note about this.

> 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()).
>
That' s not the goal, as tp-qt4 is not thread-safe at all :)



Again, thanks for the review

Reviewed branch at 
http://git.collabora.co.uk/?p=user/andrunko/telepathy-qt4.git;a=shortlog;h=refs/heads/shared

BR
Andrunko