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.
Created attachment 247168 [details] Patch
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 :(
(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 **
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!
Committed r180535: <http://trac.webkit.org/changeset/180535>
This broke the build: https://build.webkit.org/builders/Apple%20Yosemite%20Release%20%28Build%29/builds/3236/steps/compile-webkit/logs/stdio
(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.
(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.
Can we just dump clear?
Seems like "= nullptr" is sufficient.
(In reply to comment #10) > Seems like "= nullptr" is sufficient. Yes, but the other smart pointer implementations do provide a 'clear' method.
(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.
(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.