Bug 177389 – WeakPtrFactory should allow downcasting

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Jiewen Tan]

WeakPtrFactory should allow downcasting, i.e. creating a weak pointer of the current class' derived classes.

[Radar WebKit Bug Importer]

<rdar://problem/34604174>

[Jiewen Tan]

Geoff's proposal:

The Problem we have currently:
WeakPtr<Element> ptr = parent.createWeakPtr(); // fails: returns WeakPtr<Node>
WeakPtr<Element> ptr = parent.createWeakPtr<Element>(); // succeeds: returns WeakPtr<Element>

Solution is something like this:
   template<typename U> WeakPtr<U> createWeakPtr() const { return WeakPtr<U>(static_reference_cast<U>(m_ref); }

[zalan]

Created attachment 321758 [details]
Patch

[Jiewen Tan]

Comment on attachment 321758 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=321758&action=review

Thank you for making this patch. Have one question listed below. Could you explain it to me?

> Source/WTF/wtf/WeakPtr.h:108
> +        return WeakPtr<U>(Ref<WeakReference<U>>(static_cast<WeakReference<U>&>(*m_ref)));

No sure why static_cast works here. Could you elaborate it a little bit?

[Geoffrey Garen]

> > Source/WTF/wtf/WeakPtr.h:108
> > +        return WeakPtr<U>(Ref<WeakReference<U>>(static_cast<WeakReference<U>&>(*m_ref)));
> 
> No sure why static_cast works here. Could you elaborate it a little bit?

Yeah, I didn't write it quite right here. I think Zalan has a working copy of this function now.

[Geoffrey Garen]

> > Source/WTF/wtf/WeakPtr.h:108
> > +        return WeakPtr<U>(Ref<WeakReference<U>>(static_cast<WeakReference<U>&>(*m_ref)));
> 
> No sure why static_cast works here. Could you elaborate it a little bit?

Yeah, actually, this is wrong. It only worked in this patch because T and U are equal across all invocations. But when T and U are not equal, static casting WeakReference<T>& to WeakReference<U>& is an error.

So, we need a better version of this function.

[zalan]

(In reply to Geoffrey Garen from comment #6)
> > > Source/WTF/wtf/WeakPtr.h:108
> > > +        return WeakPtr<U>(Ref<WeakReference<U>>(static_cast<WeakReference<U>&>(*m_ref)));
> > 
> > No sure why static_cast works here. Could you elaborate it a little bit?
> 
> Yeah, actually, this is wrong. It only worked in this patch because T and U
> are equal across all invocations. But when T and U are not equal, static
> casting WeakReference<T>& to WeakReference<U>& is an error.
> 
> So, we need a better version of this function.
Yea, and I was going to work on that. This was more of the first iteration, hence not landed yet.

[Geoffrey Garen]

I guess there are two ways to fix this:

We could add a reinterpret_cast that static_asserts the conversion between T* and U* is OK.

Or we could add a template parameter to WeakPtr, and say that WeakPtr points to WeakReference<T>, and casts to U* when you get its contents.

The second option is probably more in line with idiomatic C++, so if it doesn't make WeakPtr construction too annoying, it might be preferred.

[Jiewen Tan]

Created attachment 321940 [details]
Patch

[Build Bot]

Attachment 321940 [details] did not pass style-queue:


ERROR: Source/WTF/wtf/WeakPtr.h:68:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/WeakPtr.h:69:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
Total errors found: 2 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Jiewen Tan]

Comment on attachment 321940 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=321940&action=review

> Tools/TestWebKitAPI/Tests/WTF/WeakPtr.cpp:215
> +    WeakPtr<Derived, Base> derivedWeakPtr;

Ryosuke and I are kind of worrying this format will be annoying to use. However, I don't know how it could be prevented, i.e. using WeakPtr<Derived> instead of WeakPtr<Derived, Base>. If anyone has any ideas, please tell me. Appreciate it.

[Geoffrey Garen]

Comment on attachment 321940 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=321940&action=review

> Source/WTF/wtf/WeakPtr.h:88
> +        static_assert((!std::is_class<U>::value && !std::is_class<T>::value) || std::is_base_of<U, T>::value, "U should be a base class of T");

There's no need for this assertion because it's built into the static_cast. The value of the double template parameter solution is that you don't need any assertions: The language guarantees correctness.

[Geoffrey Garen]

Comment on attachment 321940 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=321940&action=review

>> Source/WTF/wtf/WeakPtr.h:88
>> +        static_assert((!std::is_class<U>::value && !std::is_class<T>::value) || std::is_base_of<U, T>::value, "U should be a base class of T");
> 
> There's no need for this assertion because it's built into the static_cast. The value of the double template parameter solution is that you don't need any assertions: The language guarantees correctness.

...built into the static_cast in the getter.

>> Tools/TestWebKitAPI/Tests/WTF/WeakPtr.cpp:215
>> +    WeakPtr<Derived, Base> derivedWeakPtr;
> 
> Ryosuke and I are kind of worrying this format will be annoying to use. However, I don't know how it could be prevented, i.e. using WeakPtr<Derived> instead of WeakPtr<Derived, Base>. If anyone has any ideas, please tell me. Appreciate it.

If this is too annoying, we can use the reinterpret_cast solution:

template<typename T, typename U>
WeakReference<T>& static_weak_reference_cast(WeakReference<U>& weakReference)
{
    static_assert(std::is_convertible<T*, U*>::value, "U* must be convertible to T*");
    return reinterpret_cast<WeakReference <T>&>(weakReference);
}

Then createWeakPtr would static_weak_reference_cast m_ref when creating a WeakPtr.

[Jiewen Tan]

Comment on attachment 321940 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=321940&action=review

Thanks Geoff for reviewing the patch. I will upload a patch addressing comments in this patch and then upload another that uses the first solution. Then we can decide which one to go.

>>> Source/WTF/wtf/WeakPtr.h:88
>>> +        static_assert((!std::is_class<U>::value && !std::is_class<T>::value) || std::is_base_of<U, T>::value, "U should be a base class of T");
>> 
>> There's no need for this assertion because it's built into the static_cast. The value of the double template parameter solution is that you don't need any assertions: The language guarantees correctness.
> 
> ...built into the static_cast in the getter.

Removed.

>>> Tools/TestWebKitAPI/Tests/WTF/WeakPtr.cpp:215
>>> +    WeakPtr<Derived, Base> derivedWeakPtr;
>> 
>> Ryosuke and I are kind of worrying this format will be annoying to use. However, I don't know how it could be prevented, i.e. using WeakPtr<Derived> instead of WeakPtr<Derived, Base>. If anyone has any ideas, please tell me. Appreciate it.
> 
> If this is too annoying, we can use the reinterpret_cast solution:
> 
> template<typename T, typename U>
> WeakReference<T>& static_weak_reference_cast(WeakReference<U>& weakReference)
> {
>     static_assert(std::is_convertible<T*, U*>::value, "U* must be convertible to T*");
>     return reinterpret_cast<WeakReference <T>&>(weakReference);
> }
> 
> Then createWeakPtr would static_weak_reference_cast m_ref when creating a WeakPtr.

Sure. The first solution will definitely resolve this issue for us. I was trying to upload two patches, one for each, yesterday, but was not able to accomplish that. Building for WTF is too costly, I am wondering if it is possible to just build WTF and corresponding API tests.

[Jiewen Tan]

Created attachment 321978 [details]
Second Solution

[Build Bot]

Attachment 321978 [details] did not pass style-queue:


ERROR: Source/WTF/wtf/WeakPtr.h:66:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/WeakPtr.h:67:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/WeakPtr.h:83:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
Total errors found: 3 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Jiewen Tan]

(In reply to Jiewen Tan from comment #14)
> Comment on attachment 321940 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=321940&action=review
> 
> Thanks Geoff for reviewing the patch. I will upload a patch addressing
> comments in this patch and then upload another that uses the first solution.
> Then we can decide which one to go.
> 
> >>> Source/WTF/wtf/WeakPtr.h:88
> >>> +        static_assert((!std::is_class<U>::value && !std::is_class<T>::value) || std::is_base_of<U, T>::value, "U should be a base class of T");
> >> 
> >> There's no need for this assertion because it's built into the static_cast. The value of the double template parameter solution is that you don't need any assertions: The language guarantees correctness.
> > 
> > ...built into the static_cast in the getter.
> 
> Removed.
> 
> >>> Tools/TestWebKitAPI/Tests/WTF/WeakPtr.cpp:215
> >>> +    WeakPtr<Derived, Base> derivedWeakPtr;
> >> 
> >> Ryosuke and I are kind of worrying this format will be annoying to use. However, I don't know how it could be prevented, i.e. using WeakPtr<Derived> instead of WeakPtr<Derived, Base>. If anyone has any ideas, please tell me. Appreciate it.
> > 
> > If this is too annoying, we can use the reinterpret_cast solution:
> > 
> > template<typename T, typename U>
> > WeakReference<T>& static_weak_reference_cast(WeakReference<U>& weakReference)
> > {
> >     static_assert(std::is_convertible<T*, U*>::value, "U* must be convertible to T*");
> >     return reinterpret_cast<WeakReference <T>&>(weakReference);
> > }
> > 
> > Then createWeakPtr would static_weak_reference_cast m_ref when creating a WeakPtr.
> 
> Sure. The first solution will definitely resolve this issue for us. I was
> trying to upload two patches, one for each, yesterday, but was not able to
> accomplish that. Building for WTF is too costly, I am wondering if it is
> possible to just build WTF and corresponding API tests.

Hmm, I could build WTF and TestWebKitAPI separately.

[Jiewen Tan]

Created attachment 321989 [details]
First Solution

[Build Bot]

Attachment 321989 [details] did not pass style-queue:


ERROR: Source/WTF/wtf/WeakPtr.h:71:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
Total errors found: 1 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Jiewen Tan]

Comment on attachment 321989 [details]
First Solution

View in context: https://bugs.webkit.org/attachment.cgi?id=321989&action=review

> Tools/TestWebKitAPI/Tests/WTF/WeakPtr.cpp:228
> +        EXPECT_EQ(derivedWeakPtr->foo(), dummy1);

It should be derivedPtr.

[Jiewen Tan]

Comment on attachment 321978 [details]
Second Solution

View in context: https://bugs.webkit.org/attachment.cgi?id=321978&action=review

> Tools/TestWebKitAPI/Tests/WTF/WeakPtr.cpp:228
> +        EXPECT_EQ(derivedWeakPtr->foo(), dummy1);

It should be derivedPtr.

[Ryosuke Niwa]

Comment on attachment 321989 [details]
First Solution

View in context: https://bugs.webkit.org/attachment.cgi?id=321989&action=review

> Tools/TestWebKitAPI/Tests/WTF/WeakPtr.cpp:215
> +    WeakPtr<Base> baseWeakPtr;
> +    WeakPtr<Derived> derivedWeakPtr;

This looks much better to me.

[zalan]

(In reply to Ryosuke Niwa from comment #22)
> Comment on attachment 321989 [details]
> First Solution
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=321989&action=review
> 
> > Tools/TestWebKitAPI/Tests/WTF/WeakPtr.cpp:215
> > +    WeakPtr<Base> baseWeakPtr;
> > +    WeakPtr<Derived> derivedWeakPtr;
> 
> This looks much better to me.
So much better.

[Daniel Bates]

Comment on attachment 321989 [details]
First Solution

View in context: https://bugs.webkit.org/attachment.cgi?id=321989&action=review

>>> Tools/TestWebKitAPI/Tests/WTF/WeakPtr.cpp:215
>>> +    WeakPtr<Derived> derivedWeakPtr;
>> 
>> This looks much better to me.
> 
> So much better.

Better :)

[Geoffrey Garen]

Comment on attachment 321989 [details]
First Solution

r=me

[WebKit Commit Bot]

Comment on attachment 321989 [details]
First Solution

Clearing flags on attachment: 321989

Committed r222633: <http://trac.webkit.org/changeset/222633>