Bug 192712

Summary: clang-tidy: Fix unnecessary copy of objects for operator==() methods
Product: WebKit Reporter: David Kilzer (:ddkilzer) <ddkilzer>
Component: WebCore Misc.Assignee: David Kilzer (:ddkilzer) <ddkilzer>
Status: RESOLVED FIXED    
Severity: Normal CC: aestes, commit-queue, ews-watchlist, keith_miller, mark.lam, msaboff, sbarati, webkit-bug-importer, wenson_hsieh, ysuzuki
Priority: P2 Keywords: InRadar
Version: WebKit Local Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch v1 none

Description David Kilzer (:ddkilzer) 2018-12-14 13:06:24 PST
Running `clang-tidy -header-filter=.* -checks='-*,performance-*,-performance-noexcept-*' ...` on WebCore source files found these unnecessary object copies:

Source/WebCore/contentextensions/HashableActionList.h:56:46: warning: the const qualified parameter 'other' is copied for each invocation; consider making it a reference [performance-unnecessary-value-param]
    bool operator==(const HashableActionList other) const
                                             ^
                                            &
Source/WebCore/contentextensions/HashableActionList.h:61:46: warning: the const qualified parameter 'other' is copied for each invocation; consider making it a reference [performance-unnecessary-value-param]
    bool operator!=(const HashableActionList other) const
                                             ^
                                            &

Source/WebCore/platform/network/FormData.h:88:47: warning: the const qualified parameter 'other' is copied for each invocation; consider making it a reference [performance-unnecessary-value-param]
        bool operator==(const EncodedFileData other) const
                                              ^
                                             &
Source/WebCore/platform/network/FormData.h:152:47: warning: the const qualified parameter 'other' is copied for each invocation; consider making it a reference [performance-unnecessary-value-param]
        bool operator==(const EncodedBlobData other) const
                                              ^
                                             &
Comment 1 Radar WebKit Bug Importer 2018-12-14 13:06:52 PST
<rdar://problem/46739332>
Comment 2 David Kilzer (:ddkilzer) 2018-12-14 13:24:52 PST
I also ran this terrible hack of a command to try to find other operator==() methods that weren't using const references:

$ grep -r 'bool operator==(const' Source | egrep -v '(&[ \),]| &[a-zA-Z])'

The only interesting things it found were:

Source/JavaScriptCore/b3/air/AirAllocateRegistersByGraphColoring.cpp:        bool operator==(const InterferenceEdge other) const

Source/JavaScriptCore/runtime/JSCJSValue.h:inline bool operator==(const JSValue a, const JSCell* b) { return a == JSValue(b); }
Source/JavaScriptCore/runtime/JSCJSValue.h:inline bool operator==(const JSCell* a, const JSValue b) { return JSValue(a) == b; }

Source/WTF/wtf/text/StringView.h:inline bool operator==(const char* a, StringView b) { return equal(b, a); }

I'll fix AirAllocateRegistersByGraphColoring.cpp in this patch, but handle the others using separate patches.
Comment 3 David Kilzer (:ddkilzer) 2018-12-14 13:44:35 PST
Created attachment 357340 [details]
Patch v1
Comment 4 Keith Miller 2018-12-14 14:58:31 PST
(In reply to David Kilzer (:ddkilzer) from comment #2)
> I also ran this terrible hack of a command to try to find other operator==()
> methods that weren't using const references:
> 
> $ grep -r 'bool operator==(const' Source | egrep -v '(&[ \),]| &[a-zA-Z])'

You may also want to look for "!="

> 
> The only interesting things it found were:
> 
> Source/JavaScriptCore/b3/air/AirAllocateRegistersByGraphColoring.cpp:       
> bool operator==(const InterferenceEdge other) const
> 
> Source/JavaScriptCore/runtime/JSCJSValue.h:inline bool operator==(const
> JSValue a, const JSCell* b) { return a == JSValue(b); }
> Source/JavaScriptCore/runtime/JSCJSValue.h:inline bool operator==(const
> JSCell* a, const JSValue b) { return JSValue(a) == b; }
> 
> Source/WTF/wtf/text/StringView.h:inline bool operator==(const char* a,
> StringView b) { return equal(b, a); }
> 
> I'll fix AirAllocateRegistersByGraphColoring.cpp in this patch, but handle
> the others using separate patches.
Comment 5 WebKit Commit Bot 2018-12-14 15:23:17 PST
Comment on attachment 357340 [details]
Patch v1

Clearing flags on attachment: 357340

Committed r239237: <https://trac.webkit.org/changeset/239237>
Comment 6 WebKit Commit Bot 2018-12-14 15:23:19 PST
All reviewed patches have been landed.  Closing bug.
Comment 7 David Kilzer (:ddkilzer) 2018-12-14 19:47:25 PST
(In reply to Keith Miller from comment #4)
> (In reply to David Kilzer (:ddkilzer) from comment #2)
> > I also ran this terrible hack of a command to try to find other operator==()
> > methods that weren't using const references:
> > 
> > $ grep -r 'bool operator==(const' Source | egrep -v '(&[ \),]| &[a-zA-Z])'
> 
> You may also want to look for "!="

Thanks!  I already did, but didn't find any beyond what I mentioned in Comment #2.  :)