Bug 192666

Summary: clang-tidy: Fix unnecessary parameter copies in ParallelHelperPool.cpp
Product: WebKit Reporter: David Kilzer (:ddkilzer) <ddkilzer>
Component: Web Template FrameworkAssignee: David Kilzer (:ddkilzer) <ddkilzer>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, benjamin, cdumez, cmarcelo, commit-queue, darin, dbates, ews-watchlist, fpizlo, ggaren, mark.lam, webkit-bug-importer, ysuzuki
Priority: P2 Keywords: InRadar
Version: WebKit Local Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch v1
none
Patch v1 none

Description David Kilzer (:ddkilzer) 2018-12-13 08:37:07 PST
Running `clang-tidy -checks='-*,performance-*,-performance-noexcept-*' ...` on ParallelHelperPool.cpp found these unnecessary copies:

Source/WTF/wtf/ParallelHelperPool.cpp:36:14: warning: parameter 'pool' is passed by value and only copied once; consider moving it to avoid unnecessary copies [performance-unnecessary-value-param]
    : m_pool(pool)
             ^
             std::move( )
Source/WTF/wtf/ParallelHelperPool.cpp:61:14: warning: parameter 'task' is passed by value and only copied once; consider moving it to avoid unnecessary copies [performance-unnecessary-value-param]
    m_task = task;
             ^
             std::move( )
Source/WTF/wtf/ParallelHelperPool.cpp:86:13: warning: parameter 'task' is passed by value and only copied once; consider moving it to avoid unnecessary copies [performance-unnecessary-value-param]
    setTask(task);
            ^
            std::move( )
Source/WTF/wtf/ParallelHelperPool.cpp:107:64: warning: the parameter 'task' is copied for each invocation but only used as a const reference; consider making it a const reference [performance-unnecessary-value-param]
void ParallelHelperClient::runTask(RefPtr<SharedTask<void ()>> task)
                                                               ^
                                   const                      &
Comment 1 Radar WebKit Bug Importer 2018-12-13 08:37:48 PST
<rdar://problem/46697952>
Comment 2 David Kilzer (:ddkilzer) 2018-12-13 08:46:41 PST
Created attachment 357233 [details]
Patch v1
Comment 3 EWS Watchlist 2018-12-13 08:48:33 PST
Attachment 357233 [details] did not pass style-queue:


ERROR: Source/WTF/wtf/ParallelHelperPool.h:137:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/ParallelHelperPool.h:153:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/ParallelHelperPool.h:173:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/ParallelHelperPool.cpp:57:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/ParallelHelperPool.cpp:84:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/ParallelHelperPool.cpp:107:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 6 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 4 David Kilzer (:ddkilzer) 2018-12-13 09:10:58 PST
Comment on attachment 357233 [details]
Patch v1

View in context: https://bugs.webkit.org/attachment.cgi?id=357233&action=review

> Source/WTF/wtf/ParallelHelperPool.cpp:86
>      setTask(task);

Oops!  Forgot the WTFMove() here.
Comment 5 David Kilzer (:ddkilzer) 2018-12-13 09:11:32 PST
Created attachment 357235 [details]
Patch v1
Comment 6 EWS Watchlist 2018-12-13 09:13:00 PST
Attachment 357235 [details] did not pass style-queue:


ERROR: Source/WTF/wtf/ParallelHelperPool.h:137:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/ParallelHelperPool.h:153:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/ParallelHelperPool.h:173:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/ParallelHelperPool.cpp:57:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/ParallelHelperPool.cpp:84:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/ParallelHelperPool.cpp:107:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 6 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 7 Alex Christensen 2018-12-13 11:19:33 PST
Comment on attachment 357235 [details]
Patch v1

r=me
Is there a way to see the unnecessary String copies?
Comment 8 David Kilzer (:ddkilzer) 2018-12-13 12:15:51 PST
(In reply to Alex Christensen from comment #7)
> Comment on attachment 357235 [details]
> Patch v1
> 
> r=me
> Is there a way to see the unnecessary String copies?

In a nutshell:

1. Build your own llvm/clang.
2. Build WebKit and save the full build output to a file.
3. Write a script to take the build output and "replay" the compiler commands with clang-tidy.  (I have a script attached to <rdar://problem/46556354> to do this.  I need to clean it up before landing in the WebKit repo, if the powers that be are okay with that since it requires building your own clang to use, or use a clang distro that includes clang-tidy.)
4. Run the script, then fix the issues it finds.  (I'm doing this now for JavaScriptCore and WebCore.)
Comment 9 WebKit Commit Bot 2018-12-13 12:49:19 PST
Comment on attachment 357235 [details]
Patch v1

Clearing flags on attachment: 357235

Committed r239177: <https://trac.webkit.org/changeset/239177>
Comment 10 WebKit Commit Bot 2018-12-13 12:49:21 PST
All reviewed patches have been landed.  Closing bug.