[Telepathy] tq-qt4 refcount proposal

[Jonathon Jongsma]

Andre Moreira Magalhaes wrote:
>> [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

OK, you did change the signature of the constructor to the same form used by 
boost::shared_ptr, but it doesn't appear that you made any changes internally to remember 
this type, so I'm not sure you actually gained anything by making this change.  In the 
case I mentioned above, I believe you will still leak.  It's probably best to either go 
back to the original signature and make the docs clear about this limitation, or go in all 
the way and implement the black magic from boost.

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

OK, I notice that you removed this constructor, which is good, but I didn't see any added 
API for doing the casting properly.  For example, 
Glib::RefPtr<T>::cast_dynamic(RefPtr<U>).  This is mostly syntactic sugar, but I think 
it's worthwhile since look how ugly your code got after making this change:

-    mChan = pc->channel();
+    mChan = StreamedMediaChannelPtr(
+            dynamic_cast<StreamedMediaChannel*>(pc->channel().data()));

Whereas, if you added some casting helpers, you could make it a little bit more palatable. 
Here's how it would look with Glib::RefPtr-style casting helpers:
     mChan = StreamedMediaChannelPtr::cast_dynamic(pc->channel());
or boost-style:
     mChan = dynamic_pointer_cast<StreamedMediaChannel>(pc->channel());

I think either of these two options look considerably nicer than the code above, but 
that's partly personal taste, I suppose.

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

Yes, that's fine.  just wanted to make sure the decision was considered and intentional.

So the two issues above are the only issues remaining.  Neither of them are critical 
flaws, but I think the class could be improved by addressing them.

Hope that helps.

-- 
Jonathon Jongsma
Collabora Ltd