Bug 192710 – clang-tidy: Fix unnecessary copy of AtomicString each time one is logged

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Template Framework

[David Kilzer (:ddkilzer)]

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       &

[Radar WebKit Bug Importer]

<rdar://problem/46738962>

[David Kilzer (:ddkilzer)]

Created attachment 357332 [details]
Patch v1

[Eric Carlson]

Comment on attachment 357332 [details]
Patch v1

Nice fix, thank you!

[WebKit Commit Bot]

Comment on attachment 357332 [details]
Patch v1

Clearing flags on attachment: 357332

Committed r239232: <https://trac.webkit.org/changeset/239232>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Don Olmstead]

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.

[Darin Adler]

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.

[David Kilzer (:ddkilzer)]

(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) { }

[Darin Adler]

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.