WTF::WeakPtr should rename 'forget' to 'clear', and support assignment from nullptr

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Brent Fulgham]

After some bike shedding, we decided that:

1. The new "forget" method added in Bug 141923 should be renamed "clear".
2. We really should support assignment from nullptr.

[Brent Fulgham]

Created attachment 247168 [details]
Patch

[Myles C. Maxfield]

Comment on attachment 247168 [details]
Patch

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

> Source/WTF/wtf/WeakPtr.h:103
> +    WeakPtr& operator=(std::nullptr_t) { m_ref = WeakReference<T>::create(nullptr); return *this; }

Is it valuable to use the argument here? (for performance, obviously correctness is right)

> Tools/TestWebKitAPI/Tests/WTF/WeakPtr.cpp:183
> +    weakPtr7 = nullptr;

I wish there was a way we could test compilation failures, so we could make sure that operator=() never works with raw pointers :(

[Brent Fulgham]

(In reply to comment #2)
> Comment on attachment 247168 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=247168&action=review
> 
> > Source/WTF/wtf/WeakPtr.h:103
> > +    WeakPtr& operator=(std::nullptr_t) { m_ref = WeakReference<T>::create(nullptr); return *this; }
> 
> Is it valuable to use the argument here? (for performance, obviously
> correctness is right)

I'm not sure. Maybe Anders can tell us... But since he suggested the implementation I used, I sort of doubt that it would be better ;-)
 
> > Tools/TestWebKitAPI/Tests/WTF/WeakPtr.cpp:183
> > +    weakPtr7 = nullptr;
> 
> I wish there was a way we could test compilation failures, so we could make
> sure that operator=() never works with raw pointers :(

I did try adding this to the end of the WeakPtr.cpp test file:

    int doNotCopyMe = 152;
    weakPtr7 = &doNotCopyMe;

The compiler properly complained:

/Volumes/Data/Projects/WebKit/OpenSource/Tools/TestWebKitAPI/Tests/WTF/WeakPtr.cpp:187:14: error: no viable overloaded '='
    weakPtr7 = &doNotCopyMe;
    ~~~~~~~~ ^ ~~~~~~~~~~~~
In file included from /Volumes/Data/Projects/WebKit/OpenSource/Tools/TestWebKitAPI/Tests/WTF/WeakPtr.cpp:29:
/Volumes/Data/Projects/WebKit/OpenSource/WebKitBuild/Debug/usr/local/include/wtf/WeakPtr.h:101:14: note: candidate function not viable: no known conversion from 'int *' to 'const WTF::WeakPtr<int>' for 1st argument
    WeakPtr& operator=(const WeakPtr& o) { m_ref = o.m_ref.copyRef(); return *this; }
             ^
/Volumes/Data/Projects/WebKit/OpenSource/WebKitBuild/Debug/usr/local/include/wtf/WeakPtr.h:103:14: note: candidate function not viable: no known conversion from 'int *' to 'std::nullptr_t' (aka 'nullptr_t') for 1st argument
    WeakPtr& operator=(std::nullptr_t) { m_ref = WeakReference<T>::create(nullptr); return *this; }
             ^
/Volumes/Data/Projects/WebKit/OpenSource/WebKitBuild/Debug/usr/local/include/wtf/WeakPtr.h:102:35: note: candidate template ignored: could not match 'WeakPtr<type-parameter-0-0>' against 'int *'
    template<typename U> WeakPtr& operator=(const WeakPtr<U>& o) { m_ref = o.m_ref.copyRef(); return *this; }
                                  ^
1 error generated.

** BUILD FAILED **

[Brent Fulgham]

Comment on attachment 247168 [details]
Patch

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

>>> Source/WTF/wtf/WeakPtr.h:103
>>> +    WeakPtr& operator=(std::nullptr_t) { m_ref = WeakReference<T>::create(nullptr); return *this; }
>> 
>> Is it valuable to use the argument here? (for performance, obviously correctness is right)
> 
> I'm not sure. Maybe Anders can tell us... But since he suggested the implementation I used, I sort of doubt that it would be better ;-)

I caught Anders in the hallway, and he say NO NEED!

[Brent Fulgham]

Committed r180535: <http://trac.webkit.org/changeset/180535>

[Alexey Proskuryakov]

This broke the build: https://build.webkit.org/builders/Apple%20Yosemite%20Release%20%28Build%29/builds/3236/steps/compile-webkit/logs/stdio

[Ryosuke Niwa]

(In reply to comment #6)
> This broke the build:
> https://build.webkit.org/builders/Apple%20Yosemite%20Release%20%28Build%29/
> builds/3236/steps/compile-webkit/logs/stdio

Fixed in https://trac.webkit.org/changeset/180540.

[Anders Carlsson]

(In reply to comment #4)
> Comment on attachment 247168 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=247168&action=review
> 
> >>> Source/WTF/wtf/WeakPtr.h:103
> >>> +    WeakPtr& operator=(std::nullptr_t) { m_ref = WeakReference<T>::create(nullptr); return *this; }
> >> 
> >> Is it valuable to use the argument here? (for performance, obviously correctness is right)
> > 
> > I'm not sure. Maybe Anders can tell us... But since he suggested the implementation I used, I sort of doubt that it would be better ;-)
> 
> I caught Anders in the hallway, and he say NO NEED!

To clarify, since nullptr_t is an "empty type", meaning that it has no members etc, it won't even be passed in a register or on the stack! It's only used for overload resolution.

[Darin Adler]

Can we just dump clear?

[Darin Adler]

Seems like "= nullptr" is sufficient.

[Brent Fulgham]

(In reply to comment #10)
> Seems like "= nullptr" is sufficient.

Yes, but the other smart pointer implementations do provide a 'clear' method.

[Chris Dumez]

(In reply to comment #11)
> (In reply to comment #10)
> > Seems like "= nullptr" is sufficient.
> 
> Yes, but the other smart pointer implementations do provide a 'clear' method.

But we prefer "= nullptr;" in new code I believe, for consistency with std::unique_ptr.

[Darin Adler]

(In reply to comment #11)
> Yes, but the other smart pointer implementations do provide a 'clear' method.

Ours do, but std::unique_ptr does not. I think eventually we will want to remove our clear functions. We made those back when there was no nullptr in the language.