Summary: | clang-tidy: Fix unnecessary copy of AtomicString each time one is logged | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | David Kilzer (:ddkilzer) <ddkilzer> | ||||
Component: | Web Template Framework | Assignee: | David Kilzer (:ddkilzer) <ddkilzer> | ||||
Status: | RESOLVED FIXED | ||||||
Severity: | Normal | CC: | aestes, benjamin, cdumez, cmarcelo, commit-queue, darin, dbates, don.olmstead, eric.carlson, ews-watchlist, webkit-bug-importer | ||||
Priority: | P2 | Keywords: | InRadar | ||||
Version: | WebKit Local Build | ||||||
Hardware: | Unspecified | ||||||
OS: | Unspecified | ||||||
Attachments: |
|
Description
David Kilzer (:ddkilzer)
2018-12-14 12:51:16 PST
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. |