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 &
<rdar://problem/46738962>
Created attachment 357332 [details] Patch v1
Comment on attachment 357332 [details] Patch v1 Nice fix, thank you!
Comment on attachment 357332 [details] Patch v1 Clearing flags on attachment: 357332 Committed r239232: <https://trac.webkit.org/changeset/239232>
All reviewed patches have been landed. Closing bug.
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.
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.
(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) { }
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.