Drop RefPtr::clear() method as we are instructing people to use "= nullptr" pattern instead.
Created attachment 256024 [details] WIP Patch
Created attachment 256026 [details] WIP Patch
Created attachment 256027 [details] WIP Patch
Created attachment 256031 [details] Patch
I think this is a super great thing to do, and even started poking around it myself. I did notice that the mechanics behind operator=(T*) and .clear() are subtly different, and I wasn't able to immediately convince myself there was 0-risk of a change in performance characteristics. I brought this up to Chris on IRC, and I believe he's digging in more detail.
If there is any performance cost to using operator= we can overload it for std::nullptr_t.
(In reply to comment #6) > If there is any performance cost to using operator= we can overload it for > std::nullptr_t. Right. I'll benchmark locally to confirm if there is a noticeable performance difference between clear() and our current operator=(T*).
Created attachment 256126 [details] WIP Patch
Attachment 256126 [details] did not pass style-queue: ERROR: Source/WebKit/Storage/StorageAreaSync.cpp:91: One space before end of line comments [whitespace/comments] [5] Total errors found: 1 in 187 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 256127 [details] Patch
Created attachment 256129 [details] Patch
Created attachment 256130 [details] Patch
Created attachment 256131 [details] Patch
Created attachment 256132 [details] Patch
Created attachment 256133 [details] Patch
Created attachment 256134 [details] Patch
Created attachment 256135 [details] Patch
Created attachment 256136 [details] Patch
GETTING SO CLOSE! Damned Windows build!
(In reply to comment #19) > GETTING SO CLOSE! > > Damned Windows build! No no, the Windows EWS issue is unrelated :) The patch is ready.
Really? https://webkit-queues.appspot.com/results/5659624707457024 c:\cygwin\home\buildbot\webkit\source\webcore\dom\Position.h(95): error C2039: 'clear' : is not a member of 'WTF::RefPtr<WebCore::Node>' (..\page\win\EventHandlerWin.cpp) [C:\cygwin\home\buildbot\WebKit\Source\WebCore\WebCore.vcxproj\WebCore.vcxproj] c:\cygwin\home\buildbot\webkit\source\webcore\dom\RangeBoundaryPoint.h(115): error C2039: 'clear' : is not a member of 'WTF::RefPtr<WebCore::Node>' (..\page\win\EventHandlerWin.cpp) [C:\cygwin\home\buildbot\WebKit\Source\WebCore\WebCore.vcxproj\WebCore.vcxproj] c:\cygwin\home\buildbot\webkit\source\webcore\dom\RangeBoundaryPoint.h(117): error C2593: 'operator =' is ambiguous (..\page\win\EventHandlerWin.cpp) [C:\cygwin\home\buildbot\WebKit\Source\WebCore\WebCore.vcxproj\WebCore.vcxproj] etc etc.
(In reply to comment #21) > Really? > > https://webkit-queues.appspot.com/results/5659624707457024 > > c:\cygwin\home\buildbot\webkit\source\webcore\dom\Position.h(95): error > C2039: 'clear' : is not a member of 'WTF::RefPtr<WebCore::Node>' > (..\page\win\EventHandlerWin.cpp) > [C:\cygwin\home\buildbot\WebKit\Source\WebCore\WebCore.vcxproj\WebCore. > vcxproj] > c:\cygwin\home\buildbot\webkit\source\webcore\dom\RangeBoundaryPoint.h(115): > error C2039: 'clear' : is not a member of 'WTF::RefPtr<WebCore::Node>' > (..\page\win\EventHandlerWin.cpp) > [C:\cygwin\home\buildbot\WebKit\Source\WebCore\WebCore.vcxproj\WebCore. > vcxproj] > c:\cygwin\home\buildbot\webkit\source\webcore\dom\RangeBoundaryPoint.h(117): > error C2593: 'operator =' is ambiguous (..\page\win\EventHandlerWin.cpp) > [C:\cygwin\home\buildbot\WebKit\Source\WebCore\WebCore.vcxproj\WebCore. > vcxproj] > > etc etc. Weird, I missed those when I looked. I'll fix them, thanks.
Created attachment 256139 [details] Patch
Comment on attachment 256139 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=256139&action=review > Source/JavaScriptCore/parser/Parser.cpp:494 > + lastPattern = TreeDestructuringPattern(0); Is this one necessary? > Source/JavaScriptCore/parser/Parser.cpp:897 > + pattern = TreeDestructuringPattern(0); Or this one?
Comment on attachment 256139 [details] Patch r+ but please make sure it won't break anything else.
(In reply to comment #24) > Comment on attachment 256139 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=256139&action=review > > > Source/JavaScriptCore/parser/Parser.cpp:494 > > + lastPattern = TreeDestructuringPattern(0); > > Is this one necessary? > > > Source/JavaScriptCore/parser/Parser.cpp:897 > > + pattern = TreeDestructuringPattern(0); > > Or this one? The reason I did this is because the functions are templated and depending on the template parameter, lastPattern / pattern can be either an int or a RefPtr. Therefore, using eight "= 0;" or "= nullptr;" wouldn't build. Calling the TreeDestructuringPattern constructor explicitly is the simplest workaround I found.
Comment on attachment 256139 [details] Patch Clearing flags on attachment: 256139 Committed r186279: <http://trac.webkit.org/changeset/186279>
All reviewed patches have been landed. Closing bug.