Let's kill SupportsWeakPtr (was Re: [chromium-dev] Let's kill NotificationService)

[Wez]

We got as far as expressing a preference in favour of using WeakPtrFactory over SupportsWeakPtr, but didn't go as far as phasing it out.

Based on how easily it can in general be replaced by WeakPtrFactory, I still think it's worth phasing out.

Wez

On 7 August 2013 10:56, Ryan Sleevi <rsl...@chromium.org> wrote:

On Wed, Aug 7, 2013 at 10:54 AM, Darin Fisher <da...@chromium.org> wrote:
> I thought we had decided to phase out SupportsWeakPtr.  Did that idea
> fizzle?
>
> -Darin
>

I don't recall reaching a conclusion to phase it out. It's a useful
class/abstraction, but as mentioned, it makes more sense within inner
classes / implementation helpers / implementation classes - just not
on 'public' classes.

>
> On Wed, Aug 7, 2013 at 10:51 AM, Ryan Sleevi <rsl...@chromium.org> wrote:
>>
>> On Wed, Aug 7, 2013 at 8:25 AM, Jeffrey Yasskin <jyas...@chromium.org>
>> wrote:
>> > On Wed, Aug 7, 2013 at 2:25 AM, William Chan (陈智昌)
>> > <will...@chromium.org> wrote:
>> >> On Wed, Aug 7, 2013 at 2:23 AM, William Chan (陈智昌)
>> >> <will...@chromium.org>
>> >> wrote:
>> >>>
>> >>> I don't have time to write up a rant on why NotificationService is
>> >>> bad,
>> >>> but I'm pleased to see there's a lot of momentum here so I probably
>> >>> don't
>> >>> need to. So, +1 on removing NotificationService.
>> >>>
>> >>> On Wed, Aug 7, 2013 at 1:40 AM, Drew Wilson <atwi...@chromium.org>
>> >>> wrote:
>> >>>>
>> >>>> In the current world we use notifications. In the new world we would:
>> >>>>
>> >>>> a) Change class B so it SupportsWeakPtr<>.
>> >>>
>> >>>
>> >>> Please avoid using WeakPtr, and especially SupportsWeakPtr, if
>> >>> possible.
>> >>
>> >>
>> >> Er, quick clarification on this one, I mean avoid passing WeakPtr<T>
>> >> outside
>> >> of T* (as opposed to using the WeakPtr<T> internally to T*, in which
>> >> case
>> >> WeakPtrFactory is much better).
>> >
>> > In many cases, we wind up with an apparent choice between
>> > SupportsWeakPtr (or its equivalent) and an Observer method
>> > ObserveeDestroyed(), which reimplements the functionality of WeakPtr.
>> > IMO, the dedicated class is easier to read and results in simpler
>> > code, so we should prefer it.
>> >
>> > I agree that designing simple "X outlives Y and has no pointers to Y"
>> > relationships is better (so we don't need either), but we need to make
>> > that the rule. We shouldn't discourage WeakPtr (the simplest way of
>> > coping with complex lifetimes) while allowing more complex ways of
>> > doing the same thing.
>> >
>> > Jeffrey
>> >
>> > P.S. Re Drew's "I want to get a WeakPtr to B", I believe Will's point
>> > is that you _shouldn't_ "want to get a WeakPtr to B". :)
>>
>> From discussions with Will on this point, I think the choice between
>> SupportsWeakPtr and ObserveeDestroyed() are a false choice - what you
>> really what is contractual lifetimes between objects. I also think
>> using WeakPtr<> to simulate the FooDestroyed case is actually fairly
>> dangerous, in that WeakPtr<> has additional pre-conditions that a
>> *Destroyed observer doesn't necessarily have.
>>
>> SupportsWeakPtr (which exposes your WeakPtr to arbitrary callers) is,
>> arguably, as much a design anti-pattern as public RefCounting. It's
>> not that WeakPtrs are inherently bad, just like RefCounting isn't
>> inherently bad, but it can make it much more difficult to reason about
>> the correctness of the code.
>>
>> If you're going to use WeakPtrs (and they're extremely handy), the
>> 'preferred' approach is that you encapulsate the WeakPtrFactory within
>> your class - so that there's a single point at which WeakPtrs can be
>> obtained - within the implementation file. Using base::Bind() and
>> friends to create 'cancellable callbacks' lets you have situations
>> where A outlives B, but B needs to do something on A and may disappear
>> while A is doing it.
>>
>> SupportsWeakPtr<> as a design pattern should only really be used in
>> private classes / implementation classes, because it offers the same
>> design constraint as having a (private) WeakPtrFactory and no (public)
>> way to vend WeakPtrs - the WeakPtr lifecycle is kept entirely within a
>> file and can be reasoned about for correctness (eg: thread affinity,
>> lifetimes, etc)
>
>

--
--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
    http://groups.google.com/a/chromium.org/group/chromium-dev

[Jeffrey Yasskin]

This is a minor issue, but WeakPtrFactory costs 1 more word of memory
than SupportsWeakPtr. If we specialize WeakPtrFactory<void> to not
store the pointer, and instead define:

template<typename T>
WeakPtr<T> WeakPtrFactory<void>::GetWeakPtr(T* ptr) {
DCHECK(ptr);
return WeakPtr<T>(weak_reference_owner_.GetRef(), ptr);
}

we'd save that pointer, and make the type more flexible at the same
time. The downside, of course, is that someone could pass an object to
GetWeakPtr() whose lifetime is not strictly longer than the
WeakPtrFactory's lifetime.

[Wez]

I don't have a good feel for whether the extra pointer is costly enough to justify that work-around.

If we did add it then it'd need to check that |ptr| matches the content of |flag_|, if present, so it's clear that a single WeakPtrFactory still can't manage more than one object.

[Jeffrey Yasskin]

On Tue, Aug 20, 2013 at 6:01 PM, Wez <w...@chromium.org> wrote:
> I don't have a good feel for whether the extra pointer is costly enough to
> justify that work-around.
>
> If we did add it then it'd need to check that |ptr| matches the content of
> |flag_|, if present, so it's clear that a single WeakPtrFactory still can't
> manage more than one object.

There is no content of |flag_|:
https://code.google.com/p/chromium/codesearch/#chromium/src/base/memory/weak_ptr.h&l=82
...

[Wez]

Oh, good point, that's on WeakPtr itself. :-/

[Jonathan Dixon]

On 20 August 2013 12:21, Jeffrey Yasskin <jyas...@chromium.org> wrote:

This is a minor issue, but WeakPtrFactory costs 1 more word of memory
than SupportsWeakPtr.

I'll bite. Aside from the abominable style violation, what's the practical downside to just allowing private inheritance for SupportsWeakPtr?

[Wez]

SupportsWeakPtr invalidates WeakPtrs to itself in the destructor, but the destructor only runs after the derived class' destructor (and those of the derived class' members).

A WeakPtrFactory member will invalidate WeakPtrs when destroyed, which will also be after the derived class' destructor, but can at least be placed so as to invalidate WeakPtrs before any other members are torn down.