Bug 192666 - clang-tidy: Fix unnecessary parameter copies in ParallelHelperPool.cpp
Summary: clang-tidy: Fix unnecessary parameter copies in ParallelHelperPool.cpp
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Template Framework (show other bugs)
Version: WebKit Local Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: David Kilzer (:ddkilzer)
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-12-13 08:37 PST by David Kilzer (:ddkilzer)
Modified: 2018-12-13 12:49 PST (History)
13 users (show)

See Also:


Attachments
Patch v1 (4.11 KB, patch)
2018-12-13 08:46 PST, David Kilzer (:ddkilzer)
no flags Details | Formatted Diff | Diff
Patch v1 (4.09 KB, patch)
2018-12-13 09:11 PST, David Kilzer (:ddkilzer)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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.