192712 2018-12-14 13:06:24 -0800 clang-tidy: Fix unnecessary copy of objects for operator==() methods 2018-12-14 19:47:25 -0800 1 1 1 Unclassified WebKit WebCore Misc. WebKit Local Build Unspecified Unspecified RESOLVED FIXED InRadar P2 Normal --- 1 ddkilzer ddkilzer aestes commit-queue ews-watchlist keith_miller mark.lam msaboff saam webkit-bug-importer wenson_hsieh ysuzuki oldest_to_newest 1488017 0 ddkilzer 2018-12-14 13:06:24 -0800 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 ^ & 1488018 1 webkit-bug-importer 2018-12-14 13:06:52 -0800 <rdar://problem/46739332> 1488026 2 ddkilzer 2018-12-14 13:24:52 -0800 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. 1488047 3 357340 ddkilzer 2018-12-14 13:44:35 -0800 Created attachment 357340 Patch v1 1488065 4 keith_miller 2018-12-14 14:58:31 -0800 (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. 1488075 5 357340 commit-queue 2018-12-14 15:23:17 -0800 Comment on attachment 357340 Patch v1 Clearing flags on attachment: 357340 Committed r239237: <https://trac.webkit.org/changeset/239237> 1488076 6 commit-queue 2018-12-14 15:23:19 -0800 All reviewed patches have been landed. Closing bug. 1488211 7 ddkilzer 2018-12-14 19:47:25 -0800 (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. :) 357340 2018-12-14 13:44:35 -0800 2018-12-14 15:23:17 -0800 Patch v1 bug-192712-20181214134434.patch text/plain 4599 ddkilzer U3VidmVyc2lvbiBSZXZpc2lvbjogMjM5MTEyCmRpZmYgLS1naXQgYS9Tb3VyY2UvSmF2YVNjcmlw dENvcmUvQ2hhbmdlTG9nIGIvU291cmNlL0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwppbmRleCBk ZmE4YzJmYjQ3MzJjNmJlYzUyZjYwOTE4MGU5M2YyNzRhZTg5ODA1Li5lMDU1ZDM3MDllOTBjOWY4 NTYwYWFjMjA1ZDVlYWY1ZGJlOGZhMjZjIDEwMDY0NAotLS0gYS9Tb3VyY2UvSmF2YVNjcmlwdENv cmUvQ2hhbmdlTG9nCisrKyBiL1NvdXJjZS9KYXZhU2NyaXB0Q29yZS9DaGFuZ2VMb2cKQEAgLTEs MyArMSwxNSBAQAorMjAxOC0xMi0xNCAgRGF2aWQgS2lsemVyICA8ZGRraWx6ZXJAYXBwbGUuY29t PgorCisgICAgICAgIGNsYW5nLXRpZHk6IEZpeCB1bm5lY2Vzc2FyeSBjb3B5IG9mIG9iamVjdHMg Zm9yIG9wZXJhdG9yPT0oKSBtZXRob2RzCisgICAgICAgIDxodHRwczovL3dlYmtpdC5vcmcvYi8x OTI3MTI+CisgICAgICAgIDxyZGFyOi8vcHJvYmxlbS80NjczOTMzMj4KKworICAgICAgICBSZXZp ZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAgICAqIGIzL2Fpci9BaXJBbGxvY2F0ZVJl Z2lzdGVyc0J5R3JhcGhDb2xvcmluZy5jcHA6CisgICAgICAgIChKU0M6OkIzOjpBaXI6Oihhbm9u eW1vdXMgbmFtZXNwYWNlKTo6QWJzdHJhY3RDb2xvcmluZ0FsbG9jYXRvcjo6SW50ZXJmZXJlbmNl RWRnZTo6b3BlcmF0b3I9PSk6CisgICAgICAgIC0gQ2hhbmdlIGFyZ3VtZW50IGZyb20gY29uc3Qg dG8gY29uc3QgcmVmZXJlbmNlIHRvIGF2b2lkIGEgY29weS4KKwogMjAxOC0xMi0xMyAgRGF2aWQg S2lsemVyICA8ZGRraWx6ZXJAYXBwbGUuY29tPgogCiAgICAgICAgIGNsYW5nLXRpZHk6IEZpeCB1 bm5lY2Vzc2FyeSBvYmplY3QgY29waWVzIGluIEphdmFTY3JpcHRDb3JlCmRpZmYgLS1naXQgYS9T b3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cgYi9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXgg ZDJkODc2OWVlNDRlMDNiZmZlYTliZjkxNzEzMTdkMGM0ZTY1ZDAxMC4uMGUwMGViYjZjMjVhM2Uw YzkxOWZlYTQzMWNjNmZlMDY0YjczODdiNCAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvQ2hh bmdlTG9nCisrKyBiL1NvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZwpAQCAtMSwzICsxLDIwIEBACisy MDE4LTEyLTE0ICBEYXZpZCBLaWx6ZXIgIDxkZGtpbHplckBhcHBsZS5jb20+CisKKyAgICAgICAg Y2xhbmctdGlkeTogRml4IHVubmVjZXNzYXJ5IGNvcHkgb2Ygb2JqZWN0cyBmb3Igb3BlcmF0b3I9 PSgpIG1ldGhvZHMKKyAgICAgICAgPGh0dHBzOi8vd2Via2l0Lm9yZy9iLzE5MjcxMj4KKyAgICAg ICAgPHJkYXI6Ly9wcm9ibGVtLzQ2NzM5MzMyPgorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9E WSAoT09QUyEpLgorCisgICAgICAgICogY29udGVudGV4dGVuc2lvbnMvSGFzaGFibGVBY3Rpb25M aXN0Lmg6CisgICAgICAgIChXZWJDb3JlOjpDb250ZW50RXh0ZW5zaW9uczo6SGFzaGFibGVBY3Rp b25MaXN0OjpvcGVyYXRvcj09IGNvbnN0KToKKyAgICAgICAgKFdlYkNvcmU6OkNvbnRlbnRFeHRl bnNpb25zOjpIYXNoYWJsZUFjdGlvbkxpc3Q6Om9wZXJhdG9yIT0gY29uc3QpOgorICAgICAgICAq IHBsYXRmb3JtL25ldHdvcmsvRm9ybURhdGEuaDoKKyAgICAgICAgKFdlYkNvcmU6OkZvcm1EYXRh RWxlbWVudDo6RW5jb2RlZEZpbGVEYXRhOjpvcGVyYXRvcj09IGNvbnN0KToKKyAgICAgICAgKFdl YkNvcmU6OkZvcm1EYXRhRWxlbWVudDo6RW5jb2RlZEJsb2JEYXRhOjpvcGVyYXRvcj09IGNvbnN0 KToKKyAgICAgICAgLSBDaGFuZ2UgYXJndW1lbnRzIGZyb20gY29uc3QgdG8gY29uc3QgcmVmZXJl bmNlIHRvIGF2b2lkCisgICAgICAgICAgY29waWVzLgorCiAyMDE4LTEyLTE0ICBEYXZpZCBLaWx6 ZXIgIDxkZGtpbHplckBhcHBsZS5jb20+CiAKICAgICAgICAgY2xhbmctdGlkeTogRml4IHVubmVj ZXNzYXJ5IG9iamVjdCBjb3BpZXMgaW4gV2ViQ29yZS9wbGF0Zm9ybS9ncmFwaGljcy9hdmZvdW5k YXRpb24vb2JqYy8KZGlmZiAtLWdpdCBhL1NvdXJjZS9KYXZhU2NyaXB0Q29yZS9iMy9haXIvQWly QWxsb2NhdGVSZWdpc3RlcnNCeUdyYXBoQ29sb3JpbmcuY3BwIGIvU291cmNlL0phdmFTY3JpcHRD b3JlL2IzL2Fpci9BaXJBbGxvY2F0ZVJlZ2lzdGVyc0J5R3JhcGhDb2xvcmluZy5jcHAKaW5kZXgg MjZhY2ZmYzc3OTI2YTY1ZTZlY2NjZjc4NmVkNmUxMjA2ODE4NTU1ZC4uYzI4MmQ1MDA1ZDY2NmNm ZGY5ZTE3ZTRhOTU2YzE2ODNlZDQ1MDIxNiAxMDA2NDQKLS0tIGEvU291cmNlL0phdmFTY3JpcHRD b3JlL2IzL2Fpci9BaXJBbGxvY2F0ZVJlZ2lzdGVyc0J5R3JhcGhDb2xvcmluZy5jcHAKKysrIGIv U291cmNlL0phdmFTY3JpcHRDb3JlL2IzL2Fpci9BaXJBbGxvY2F0ZVJlZ2lzdGVyc0J5R3JhcGhD b2xvcmluZy5jcHAKQEAgLTQ2Niw3ICs0NjYsNyBAQCBwcm90ZWN0ZWQ6CiAgICAgICAgICAgICBy ZXR1cm4gbV92YWx1ZSAmIDB4ZmZmZmZmZmY7CiAgICAgICAgIH0KIAotICAgICAgICBib29sIG9w ZXJhdG9yPT0oY29uc3QgSW50ZXJmZXJlbmNlRWRnZSBvdGhlcikgY29uc3QKKyAgICAgICAgYm9v bCBvcGVyYXRvcj09KGNvbnN0IEludGVyZmVyZW5jZUVkZ2UmIG90aGVyKSBjb25zdAogICAgICAg ICB7CiAgICAgICAgICAgICByZXR1cm4gbV92YWx1ZSA9PSBvdGhlci5tX3ZhbHVlOwogICAgICAg ICB9CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9jb250ZW50ZXh0ZW5zaW9ucy9IYXNoYWJs ZUFjdGlvbkxpc3QuaCBiL1NvdXJjZS9XZWJDb3JlL2NvbnRlbnRleHRlbnNpb25zL0hhc2hhYmxl QWN0aW9uTGlzdC5oCmluZGV4IDEwMzljMWRhNWQ2NGQ3Zjk4ZGQ2NWI4ZTg1OWMzOGZlZGEzNzEw NmQuLjg4NzU3MDNlMTllZDk0ODYxYjRjMDc4MjYwODU2YTcwOTRlMWMzMGIgMTAwNjQ0Ci0tLSBh L1NvdXJjZS9XZWJDb3JlL2NvbnRlbnRleHRlbnNpb25zL0hhc2hhYmxlQWN0aW9uTGlzdC5oCisr KyBiL1NvdXJjZS9XZWJDb3JlL2NvbnRlbnRleHRlbnNpb25zL0hhc2hhYmxlQWN0aW9uTGlzdC5o CkBAIC01MywxMiArNTMsMTIgQEAgc3RydWN0IEhhc2hhYmxlQWN0aW9uTGlzdCB7CiAgICAgYm9v bCBpc0VtcHR5VmFsdWUoKSBjb25zdCB7IHJldHVybiBzdGF0ZSA9PSBFbXB0eTsgfQogICAgIGJv b2wgaXNEZWxldGVkVmFsdWUoKSBjb25zdCB7IHJldHVybiBzdGF0ZSA9PSBEZWxldGVkOyB9CiAK LSAgICBib29sIG9wZXJhdG9yPT0oY29uc3QgSGFzaGFibGVBY3Rpb25MaXN0IG90aGVyKSBjb25z dAorICAgIGJvb2wgb3BlcmF0b3I9PShjb25zdCBIYXNoYWJsZUFjdGlvbkxpc3QmIG90aGVyKSBj b25zdAogICAgIHsKICAgICAgICAgcmV0dXJuIHN0YXRlID09IG90aGVyLnN0YXRlICYmIGFjdGlv bnMgPT0gb3RoZXIuYWN0aW9uczsKICAgICB9CiAKLSAgICBib29sIG9wZXJhdG9yIT0oY29uc3Qg SGFzaGFibGVBY3Rpb25MaXN0IG90aGVyKSBjb25zdAorICAgIGJvb2wgb3BlcmF0b3IhPShjb25z dCBIYXNoYWJsZUFjdGlvbkxpc3QmIG90aGVyKSBjb25zdAogICAgIHsKICAgICAgICAgcmV0dXJu ICEoKnRoaXMgPT0gb3RoZXIpOwogICAgIH0KZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJDb3JlL3Bs YXRmb3JtL25ldHdvcmsvRm9ybURhdGEuaCBiL1NvdXJjZS9XZWJDb3JlL3BsYXRmb3JtL25ldHdv cmsvRm9ybURhdGEuaAppbmRleCA1OWY1NWY4MzM4YmRiZTVlMjQwZmM0NWUwNTQ4NWY1YzIzZmFi YWQxLi5kN2MxZTVmYWEwZGU1MDc4M2JmYjQ3NzI0YmY1OTkxOWFiNjZmOTFkIDEwMDY0NAotLS0g YS9Tb3VyY2UvV2ViQ29yZS9wbGF0Zm9ybS9uZXR3b3JrL0Zvcm1EYXRhLmgKKysrIGIvU291cmNl L1dlYkNvcmUvcGxhdGZvcm0vbmV0d29yay9Gb3JtRGF0YS5oCkBAIC04NSw3ICs4NSw3IEBAIHN0 cnVjdCBGb3JtRGF0YUVsZW1lbnQgewogICAgICAgICAgICAgcmV0dXJuIHsgZmlsZW5hbWUuaXNv bGF0ZWRDb3B5KCksIGZpbGVTdGFydCwgZmlsZUxlbmd0aCwgZXhwZWN0ZWRGaWxlTW9kaWZpY2F0 aW9uVGltZSwgZ2VuZXJhdGVkRmlsZW5hbWUuaXNvbGF0ZWRDb3B5KCksIHNob3VsZEdlbmVyYXRl RmlsZSwgb3duc0dlbmVyYXRlZEZpbGUgfTsKICAgICAgICAgfQogICAgICAgICAKLSAgICAgICAg Ym9vbCBvcGVyYXRvcj09KGNvbnN0IEVuY29kZWRGaWxlRGF0YSBvdGhlcikgY29uc3QKKyAgICAg ICAgYm9vbCBvcGVyYXRvcj09KGNvbnN0IEVuY29kZWRGaWxlRGF0YSYgb3RoZXIpIGNvbnN0CiAg ICAgICAgIHsKICAgICAgICAgICAgIHJldHVybiBmaWxlbmFtZSA9PSBvdGhlci5maWxlbmFtZQog ICAgICAgICAgICAgICAgICYmIGZpbGVTdGFydCA9PSBvdGhlci5maWxlU3RhcnQKQEAgLTE0OSw3 ICsxNDksNyBAQCBzdHJ1Y3QgRm9ybURhdGFFbGVtZW50IHsKICAgICBzdHJ1Y3QgRW5jb2RlZEJs b2JEYXRhIHsKICAgICAgICAgVVJMIHVybDsKIAotICAgICAgICBib29sIG9wZXJhdG9yPT0oY29u c3QgRW5jb2RlZEJsb2JEYXRhIG90aGVyKSBjb25zdAorICAgICAgICBib29sIG9wZXJhdG9yPT0o Y29uc3QgRW5jb2RlZEJsb2JEYXRhJiBvdGhlcikgY29uc3QKICAgICAgICAgewogICAgICAgICAg ICAgcmV0dXJuIHVybCA9PSBvdGhlci51cmw7CiAgICAgICAgIH0K