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, saam, webkit-bug-importer, wenson_hsieh, ysuzuki
Priority: P2 Keywords: InRadar
Version: WebKit Local Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch v1 none

David Kilzer (:ddkilzer)
Reported 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 ^ &
Attachments
Patch v1 (4.49 KB, patch)
2018-12-14 13:44 PST, David Kilzer (:ddkilzer) 		no flags
DetailsFormatted DiffDiff
View All Add attachment proposed patch, testcase, etc.
Radar WebKit Bug Importer
Comment 1 2018-12-14 13:06:52 PST
<rdar://problem/46739332>
David Kilzer (:ddkilzer)
Comment 2 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.
David Kilzer (:ddkilzer)
Comment 3 2018-12-14 13:44:35 PST
Created attachment 357340 [details] Patch v1
Keith Miller
Comment 4 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.
WebKit Commit Bot
Comment 5 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>
WebKit Commit Bot
Comment 6 2018-12-14 15:23:19 PST
All reviewed patches have been landed. Closing bug.
David Kilzer (:ddkilzer)
Comment 7 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. :)
Note You need to log in before you can comment on or make changes to this bug.