192710 2018-12-14 12:51:16 -0800 clang-tidy: Fix unnecessary copy of AtomicString each time one is logged 2018-12-15 16:07:28 -0800 1 1 1 Unclassified WebKit Web Template Framework WebKit Local Build Unspecified Unspecified RESOLVED FIXED InRadar P2 Normal --- 1 ddkilzer ddkilzer aestes benjamin cdumez cmarcelo commit-queue darin dbates don.olmstead eric.carlson ews-watchlist webkit-bug-importer oldest_to_newest 1488002 0 ddkilzer 2018-12-14 12:51:16 -0800 Running `clang-tidy -header-filter=.* -checks='-*,performance-*,-performance-noexcept-*' ...` on WebCore source files found this unnecessary object copy: $BUILD_DIR/Release/usr/local/include/wtf/Logger.h:47:173: warning: the parameter 'argument' is copied for each invocation but only used as a const reference; consider making it a const reference [performance-unnecessary-value-param] template<typename U = T> static typename std::enable_if<std::is_same<typename std::remove_reference<U>::type, AtomicString>::value, String>::type toString(AtomicString argument) { return argument.string(); } ^ const & 1488004 1 webkit-bug-importer 2018-12-14 12:51:38 -0800 <rdar://problem/46738962> 1488012 2 357332 ddkilzer 2018-12-14 12:58:07 -0800 Created attachment 357332 Patch v1 1488027 3 357332 eric.carlson 2018-12-14 13:25:17 -0800 Comment on attachment 357332 Patch v1 Nice fix, thank you! 1488051 4 357332 commit-queue 2018-12-14 13:50:57 -0800 Comment on attachment 357332 Patch v1 Clearing flags on attachment: 357332 Committed r239232: <https://trac.webkit.org/changeset/239232> 1488052 5 commit-queue 2018-12-14 13:50:58 -0800 All reviewed patches have been landed. Closing bug. 1488058 6 don.olmstead 2018-12-14 14:23:34 -0800 Do we have any ideas on what clang-tidy rules should be run? I'd like to run it more often over the code base to see what it finds. Fujii is working on getting clang-cl working on Windows at which point we should be able to run it more. https://bugs.webkit.org/show_bug.cgi?id=171632 is a tracking bug for more general support for it in WebKit. 1488143 7 357332 darin 2018-12-14 17:33:59 -0800 Comment on attachment 357332 Patch v1 View in context: https://bugs.webkit.org/attachment.cgi?id=357332&action=review > Source/WTF/wtf/Logger.h:47 > + template<typename U = T> static typename std::enable_if<std::is_same<typename std::remove_reference<U>::type, AtomicString>::value, String>::type toString(const AtomicString& argument) { return argument.string(); } This "copy" here is just a ref/deref pair. It was OK to make the change, but not critical. 1488286 8 ddkilzer 2018-12-15 06:37:56 -0800 (In reply to Darin Adler from comment #7) > Comment on attachment 357332 [details] > Patch v1 > > View in context: > https://bugs.webkit.org/attachment.cgi?id=357332&action=review > > > Source/WTF/wtf/Logger.h:47 > > + template<typename U = T> static typename std::enable_if<std::is_same<typename std::remove_reference<U>::type, AtomicString>::value, String>::type toString(const AtomicString& argument) { return argument.string(); } > > This "copy" here is just a ref/deref pair. It was OK to make the change, but > not critical. I know the object holding the actual string itself (AtomicStringImpl) is not copied, but why isn't the AtomicString container copied? AtomicString(const AtomicString& other) : m_string(other.m_string) { } 1488325 9 darin 2018-12-15 16:07:28 -0800 The AtomicString is a class derived from String that adds no data members and String is just a structure with a RefPtr<StringImpl> in it. So what we are copying is a RefPtr. Just a ref/deref pair. 357332 2018-12-14 12:58:07 -0800 2018-12-14 13:50:57 -0800 Patch v1 bug-192710-20181214125807.patch text/plain 2534 ddkilzer U3VidmVyc2lvbiBSZXZpc2lvbjogMjM5MTEyCmRpZmYgLS1naXQgYS9Tb3VyY2UvV1RGL0NoYW5n ZUxvZyBiL1NvdXJjZS9XVEYvQ2hhbmdlTG9nCmluZGV4IGMwZGNhYjVlZDMyOWM4OWRlYzQwYmNm MzExYWE5YWY5MTZhMTRhOWYuLmM2NzIxYjE1OWRhODE3ZTFkZmU4MWUwYzM5NmMxMGM5ZWEwYjky MjQgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9XVEYvQ2hhbmdlTG9nCisrKyBiL1NvdXJjZS9XVEYvQ2hh bmdlTG9nCkBAIC0xLDMgKzEsMTUgQEAKKzIwMTgtMTItMTQgIERhdmlkIEtpbHplciAgPGRka2ls emVyQGFwcGxlLmNvbT4KKworICAgICAgICBjbGFuZy10aWR5OiBGaXggdW5uZWNlc3NhcnkgY29w eSBvZiBBdG9taWNTdHJpbmcgZWFjaCB0aW1lIG9uZSBpcyBsb2dnZWQKKyAgICAgICAgPGh0dHBz Oi8vd2Via2l0Lm9yZy9iLzE5MjcxMD4KKyAgICAgICAgPHJkYXI6Ly9wcm9ibGVtLzQ2NzM4OTYy PgorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgICogd3Rm L0xvZ2dlci5oOgorICAgICAgICAoV1RGOjpMb2dBcmd1bWVudDo6dG9TdHJpbmcpOiBNYWtlIGFy Z3VtZW50IGEgY29uc3QgcmVmZXJlbmNlIHRvCisgICAgICAgIGF2b2lkIHRoZSBjb3B5LgorCiAy MDE4LTEyLTEzICBEYXZpZCBLaWx6ZXIgIDxkZGtpbHplckBhcHBsZS5jb20+CiAKICAgICAgICAg Y2xhbmctdGlkeTogRml4IHVubmVjZXNzYXJ5IHBhcmFtZXRlciBjb3BpZXMgaW4gUGFyYWxsZWxI ZWxwZXJQb29sLmNwcApkaWZmIC0tZ2l0IGEvU291cmNlL1dURi93dGYvTG9nZ2VyLmggYi9Tb3Vy Y2UvV1RGL3d0Zi9Mb2dnZXIuaAppbmRleCA4ZjZlNGQzNmY3OGNkODE0ZDdjZGJlMTJmZDE1ZTJk YjQ0Nzg4YzI2Li5hODg4M2U4MzdlNGFiMmRjMzFiNzczMTVhMzZmZjIyNzhmZjJkNzVjIDEwMDY0 NAotLS0gYS9Tb3VyY2UvV1RGL3d0Zi9Mb2dnZXIuaAorKysgYi9Tb3VyY2UvV1RGL3d0Zi9Mb2dn ZXIuaApAQCAtNDQsNyArNDQsNyBAQCBzdHJ1Y3QgTG9nQXJndW1lbnQgewogICAgIHRlbXBsYXRl PHR5cGVuYW1lIFUgPSBUPiBzdGF0aWMgdHlwZW5hbWUgc3RkOjplbmFibGVfaWY8c3RkOjppc19z YW1lPFUsIGxvbmc+Ojp2YWx1ZSwgU3RyaW5nPjo6dHlwZSB0b1N0cmluZyhsb25nIGFyZ3VtZW50 KSB7IHJldHVybiBTdHJpbmc6Om51bWJlcihhcmd1bWVudCk7IH0KICAgICB0ZW1wbGF0ZTx0eXBl bmFtZSBVID0gVD4gc3RhdGljIHR5cGVuYW1lIHN0ZDo6ZW5hYmxlX2lmPHN0ZDo6aXNfc2FtZTxV LCBmbG9hdD46OnZhbHVlLCBTdHJpbmc+Ojp0eXBlIHRvU3RyaW5nKGZsb2F0IGFyZ3VtZW50KSB7 IHJldHVybiBTdHJpbmc6Om51bWJlcihhcmd1bWVudCk7IH0KICAgICB0ZW1wbGF0ZTx0eXBlbmFt ZSBVID0gVD4gc3RhdGljIHR5cGVuYW1lIHN0ZDo6ZW5hYmxlX2lmPHN0ZDo6aXNfc2FtZTxVLCBk b3VibGU+Ojp2YWx1ZSwgU3RyaW5nPjo6dHlwZSB0b1N0cmluZyhkb3VibGUgYXJndW1lbnQpIHsg cmV0dXJuIFN0cmluZzo6bnVtYmVyKGFyZ3VtZW50KTsgfQotICAgIHRlbXBsYXRlPHR5cGVuYW1l IFUgPSBUPiBzdGF0aWMgdHlwZW5hbWUgc3RkOjplbmFibGVfaWY8c3RkOjppc19zYW1lPHR5cGVu YW1lIHN0ZDo6cmVtb3ZlX3JlZmVyZW5jZTxVPjo6dHlwZSwgQXRvbWljU3RyaW5nPjo6dmFsdWUs IFN0cmluZz46OnR5cGUgdG9TdHJpbmcoQXRvbWljU3RyaW5nIGFyZ3VtZW50KSB7IHJldHVybiBh cmd1bWVudC5zdHJpbmcoKTsgfQorICAgIHRlbXBsYXRlPHR5cGVuYW1lIFUgPSBUPiBzdGF0aWMg dHlwZW5hbWUgc3RkOjplbmFibGVfaWY8c3RkOjppc19zYW1lPHR5cGVuYW1lIHN0ZDo6cmVtb3Zl X3JlZmVyZW5jZTxVPjo6dHlwZSwgQXRvbWljU3RyaW5nPjo6dmFsdWUsIFN0cmluZz46OnR5cGUg dG9TdHJpbmcoY29uc3QgQXRvbWljU3RyaW5nJiBhcmd1bWVudCkgeyByZXR1cm4gYXJndW1lbnQu c3RyaW5nKCk7IH0KICAgICB0ZW1wbGF0ZTx0eXBlbmFtZSBVID0gVD4gc3RhdGljIHR5cGVuYW1l IHN0ZDo6ZW5hYmxlX2lmPHN0ZDo6aXNfc2FtZTx0eXBlbmFtZSBzdGQ6OnJlbW92ZV9yZWZlcmVu Y2U8VT46OnR5cGUsIFN0cmluZz46OnZhbHVlLCBTdHJpbmc+Ojp0eXBlIHRvU3RyaW5nKFN0cmlu ZyBhcmd1bWVudCkgeyByZXR1cm4gYXJndW1lbnQ7IH0KICAgICB0ZW1wbGF0ZTx0eXBlbmFtZSBV ID0gVD4gc3RhdGljIHR5cGVuYW1lIHN0ZDo6ZW5hYmxlX2lmPHN0ZDo6aXNfc2FtZTx0eXBlbmFt ZSBzdGQ6OnJlbW92ZV9yZWZlcmVuY2U8VT46OnR5cGUsIFN0cmluZ0J1aWxkZXIqPjo6dmFsdWUs IFN0cmluZz46OnR5cGUgdG9TdHJpbmcoU3RyaW5nQnVpbGRlciogYXJndW1lbnQpIHsgcmV0dXJu IGFyZ3VtZW50LT50b1N0cmluZygpOyB9CiAgICAgdGVtcGxhdGU8dHlwZW5hbWUgVSA9IFQ+IHN0 YXRpYyB0eXBlbmFtZSBzdGQ6OmVuYWJsZV9pZjxzdGQ6OmlzX3NhbWU8VSwgY29uc3QgY2hhcio+ Ojp2YWx1ZSwgU3RyaW5nPjo6dHlwZSB0b1N0cmluZyhjb25zdCBjaGFyKiBhcmd1bWVudCkgeyBy ZXR1cm4gU3RyaW5nKGFyZ3VtZW50KTsgfQo=