Bug 192712 – clang-tidy: Fix unnecessary copy of objects for operator==() methods

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[David Kilzer (:ddkilzer)]

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
                                              ^
                                             &

[Radar WebKit Bug Importer]

<rdar://problem/46739332>

[David Kilzer (:ddkilzer)]

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.

[David Kilzer (:ddkilzer)]

Created attachment 357340 [details]
Patch v1

[Keith Miller]

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

[WebKit Commit Bot]

Comment on attachment 357340 [details]
Patch v1

Clearing flags on attachment: 357340

Committed r239237: <https://trac.webkit.org/changeset/239237>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[David Kilzer (:ddkilzer)]

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