[Bug 43239] Tp::SharedPtr tries to be thread-safe, but secretly isn't

bugzilla-daemon at freedesktop.org bugzilla-daemon at freedesktop.org
Tue Dec 6 16:12:33 CET 2011


https://bugs.freedesktop.org/show_bug.cgi?id=43239

Olli Salli <ollisal at gmail.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Status Whiteboard|                            |review+

--- Comment #8 from Olli Salli <ollisal at gmail.com> 2011-12-06 07:12:33 PST ---
(In reply to comment #7)
> (In reply to comment #5)
> > 1) make WeakPtr constructible from raw pointer, if there isn't some issue I'm
> > overlooking, and use it wherever sensible
> Added

Yeah looks good now.

> 
> > 2) explain in the docs, NEWS API changes section and commit message why
> > promotion from QWeakPointer is gone
> Done in the docs (NEWS will be updated when merging), but I am not really happy
> with this, suggestions are welcome.

Otherwise it's ~fine but this bit

+ * \brief The WeakPtr class holds a weak reference to a shared pointer.

is a bit weird. The reference is to a "object managed by SharedPtrs", not to a
shared pointer (the SharedPtr).

You could also mention that it's useful for breaking reference cycles which
would result from using a SharedPtr for both ends of a pair of mutually linked
objects to refer to each other.

> 
> > 3) ponder making SharedCount more of an internal implementation detail
> > (compiler checkably) than a public class, because we *can* if my memory serves
> > me correctly on the template <class X...> friend class Y; syntax
> Done
> 

Mm, couldn't you further move the entire declaration of class SharedCount here

> class TP_QT_EXPORT RefCounted
> {
>     Q_DISABLE_COPY(RefCounted)
> 
> public:

Before the start of the public section? This would hide the class completely
from external (unintended) usage. (Needs changing the three lines which refer
to SharedCount as global in WeakPtr and SharedPtr of course in addition, but
nothing else afaics?). Then the header would only declare RefCounted, WeakPtr
and SharedPtr as public types, which is what we want.

> > 4) adjust tests
> Done

Looks good now IMO.

With these, merge please.

-- 
Configure bugmail: https://bugs.freedesktop.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the QA Contact for the bug.



More information about the telepathy-bugs mailing list