Bug 192680 – clang-tidy: Fix unnecessary object copies in JavaScriptCore

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[David Kilzer (:ddkilzer)]

Running `clang-tidy -checks='-*,performance-*,-performance-noexcept-*' ...` on JavaScriptCore source files found these unnecessary object copies:

Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:6296:27: warning: the parameter 'fastTrue' is copied for each invocation but only used as a const reference; consider making it a const reference [performance-unnecessary-value-param]
    JITCompiler::JumpList fastTrue, JITCompiler::JumpList fastFalse)
                          ^
    const                &
Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:6296:59: warning: the parameter 'fastFalse' is copied for each invocation but only used as a const reference; consider making it a const reference [performance-unnecessary-value-param]
    JITCompiler::JumpList fastTrue, JITCompiler::JumpList fastFalse)
                                                          ^
                                    const                &

Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:190:166: warning: the parameter 'slowPathTarget' is copied for each invocation but only used as a const reference; consider making it a const reference [performance-unnecessary-value-param]
void SpeculativeJIT::cachedGetByIdWithThis(CodeOrigin codeOrigin, GPRReg baseGPR, GPRReg thisGPR, GPRReg resultGPR, unsigned identifierNumber, JITCompiler::JumpList slowPathTarget)
                                                                                                                                                                     ^
                                                                                                                                               const                &

Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:11291:32: warning: the parameter 'set' is copied for each invocation but only used as a const reference; consider making it a const reference [performance-unnecessary-value-param]
        RegisteredStructureSet set, const Functor& weakStructureDiscriminant)
                               ^
        const                 &

Source/JavaScriptCore/jsc.cpp:820:10: warning: the variable 'referrer' is copy-construct
ed from a const reference but is only used as const reference; consider making it a const reference [performance-unneces
sary-copy-initialization]
    auto referrer = sourceOrigin.string();
         ^
    const  &
Source/JavaScriptCore/jsc.cpp:821:10: warning: the variable 'moduleName' is copy-constructed from a const reference but is only used as const reference; consider making it a const reference [performance-unnecessary-copy-initialization]
    auto moduleName = moduleNameValue->value(exec);
         ^
    const  &
Source/JavaScriptCore/jsc.cpp:1646:29: warning: the parameter 'string' is copied for each invocation but only used as a const reference; consider making it a const reference [performance-unnecessary-value-param]
void Workers::report(String string)
                            ^
                     const &
Source/JavaScriptCore/jsc.cpp:1926:107: warning: the parameter #3 is copied for each invocation but only used as a const reference; consider making it a const reference [performance-unnecessary-value-param]
typename std::enable_if<!std::is_fundamental<ValueType>::value>::type addOption(VM&, JSObject*, Identifier, ValueType) { }
                                                                                                          ^
                                                                                                const     &
Source/JavaScriptCore/jsc.cpp:1929:124: warning: the parameter 'identifier' is copied for each invocation but only used as a const reference; consider making it a const reference [performance-unnecessary-value-param]
typename std::enable_if<std::is_fundamental<ValueType>::value>::type addOption(VM& vm, JSObject* optionsObject, Identifier identifier, ValueType value)
                                                                                                                           ^
                                                                                                                const     &
Source/JavaScriptCore/jsc.cpp:2762:24: warning: the parameter 'options' is copied for each invocation but only used as a const reference; consider making it a const reference [performance-unnecessary-value-param]
int runJSC(CommandLine options, bool isWorker, const Func& func)
                       ^
           const      &
Source/JavaScriptCore/jsc.cpp:2823:22: warning: loop variable is copied but only used as const reference; consider making it a const reference [performance-for-range-copy]
        for (CString key : compileTimeKeys)
                     ^
             const  &

Source/JavaScriptCore/assembler/testmasm.cpp:156:47: warning: the parameter 'code' is copied for each invocation but only used as a const reference; consider making it a const reference [performance-unnecessary-value-param]
T invoke(MacroAssemblerCodeRef<JSEntryPtrTag> code, Arguments... arguments)
                                              ^
         const                               &

Source/JavaScriptCore/b3/testb3.cpp:167:75: warning: the parameter 'failText' is copied for each invocation but only used as a const reference; consider making it a const reference [performance-unnecessary-value-param]
void checkDisassembly(Compilation& compilation, const Func& func, CString failText)
                                                                          ^
                                                                  const  &

[Radar WebKit Bug Importer]

<rdar://problem/46708767>

[David Kilzer (:ddkilzer)]

Created attachment 357253 [details]
Patch v1

[Mark Lam]

Comment on attachment 357253 [details]
Patch v1

LGTM if EWS bots are happy.

[David Kilzer (:ddkilzer)]

(In reply to David Kilzer (:ddkilzer) from comment #0)
> Running `clang-tidy -checks='-*,performance-*,-performance-noexcept-*' ...`
> on JavaScriptCore source files found these unnecessary object copies:

It turns out that if the warning is in a header (such as a C++ template or inline function), then you need to add the "-header-filter=.*" command-line switch to clang-tidy to see warnings from headers.  (The reason they're apparently off by default is that they can be reported more than once.)

Also, it seems like clang-tidy might treat #included source files in UnifiedSource files as "headers" and not report issues in those, either, without the "-header-filter=.*" command-line switch.

TL;DR: There will be more fixes coming to JavaScriptCore.

[WebKit Commit Bot]

Comment on attachment 357253 [details]
Patch v1

Clearing flags on attachment: 357253

Committed r239187: <https://trac.webkit.org/changeset/239187>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.