Bug 146556 – Drop RefPtr::clear() method

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Template Framework

[Chris Dumez]

Drop RefPtr::clear() method as we are instructing people to use "= nullptr" pattern instead.

[Chris Dumez]

Created attachment 256024 [details]
WIP Patch

[Chris Dumez]

Created attachment 256026 [details]
WIP Patch

[Chris Dumez]

Created attachment 256027 [details]
WIP Patch

[Chris Dumez]

Created attachment 256031 [details]
Patch

[Brady Eidson]

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.

[Darin Adler]

If there is any performance cost to using operator= we can overload it for std::nullptr_t.

[Chris Dumez]

(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*).

[Chris Dumez]

Created attachment 256126 [details]
WIP Patch

[WebKit Commit Bot]

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.

[Chris Dumez]

Created attachment 256127 [details]
Patch

[Chris Dumez]

Created attachment 256129 [details]
Patch

[Chris Dumez]

Created attachment 256130 [details]
Patch

[Chris Dumez]

Created attachment 256131 [details]
Patch

[Chris Dumez]

Created attachment 256132 [details]
Patch

[Chris Dumez]

Created attachment 256133 [details]
Patch

[Chris Dumez]

Created attachment 256134 [details]
Patch

[Chris Dumez]

Created attachment 256135 [details]
Patch

[Chris Dumez]

Created attachment 256136 [details]
Patch

[Brady Eidson]

GETTING SO CLOSE!

Damned Windows build!

[Chris Dumez]

(In reply to comment #19)
> GETTING SO CLOSE!
> 
> Damned Windows build!

No no, the Windows EWS issue is unrelated :) The patch is ready.

[Brady Eidson]

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.

[Chris Dumez]

(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.

[Chris Dumez]

Created attachment 256139 [details]
Patch

[Brady Eidson]

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?

[Brady Eidson]

Comment on attachment 256139 [details]
Patch

r+ but please make sure it won't break anything else.

[Chris Dumez]

(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.

[Chris Dumez]

Comment on attachment 256139 [details]
Patch

Clearing flags on attachment: 256139

Committed r186279: <http://trac.webkit.org/changeset/186279>

[Chris Dumez]

All reviewed patches have been landed.  Closing bug.