[Telepathy] tq-qt4 refcount proposal

[Andre Moreira Magalhaes]

Jonathon Jongsma wrote:
> 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.
>
Done, removed this black magic and back to normal constructor.

>>> [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.
>
Done, added dynamic, static and const cast helper methods

> Hope that helps.
>
It really helped, thanks

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

BR
Andrunko