Skip to content

src: apply clang-tidy rule performance-unnecessary-value-param#26042

Closed
gengjiawen wants to merge 1 commit intonodejs:masterfrom
gengjiawen:clang-tidy-perf-value-param
Closed

src: apply clang-tidy rule performance-unnecessary-value-param#26042
gengjiawen wants to merge 1 commit intonodejs:masterfrom
gengjiawen:clang-tidy-perf-value-param

Conversation

@gengjiawen
Copy link
Member

@gengjiawen gengjiawen commented Feb 11, 2019

Fix clang-tidy issue https://clang.llvm.org/extra/clang-tidy/checks/performance-unnecessary-value-param.html.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. labels Feb 11, 2019
@gengjiawen gengjiawen force-pushed the clang-tidy-perf-value-param branch from 9e5dae3 to fdd0d8e Compare February 11, 2019 10:49
refack
refack reviewed Feb 11, 2019
View reviewed changes
Copy link
Contributor

@refack refack left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm ambivalent of this change...
IMHO it has pros:

  1. Use of clang-tidy as an automated semantic tool
  2. Improve semantics of some APIs
  3. possible performance improvement

Cons:

  1. One-time use of automated tool (will regress without CI)
  2. Impair semantics of some APIs (esp. WRT to shared_ptr)
  3. performance impact non obvious. Adding <utility> has compile time impact.
  4. non K.I.S.S. use of value semantics

@refack
Copy link
Contributor

refack commented Feb 11, 2019 •
edited
Loading

@gengjiawen I like the idea of using clang-tidy. I'm not 100% sure about this specific implementation...

Lite-CI: https://ci.nodejs.org/job/node-test-pull-request-lite-pipeline/2569/

@gengjiawen
Copy link
Member Author

gengjiawen commented Feb 11, 2019

@refack Maybe start an issue to start talk about clang-tidy. I am using Clion (bundled with clang-tidy) to review nodejs cpp code :)

@refack
Copy link
Contributor

refack commented Feb 11, 2019

Maybe start an issue to start talk about clang-tidy. I am using Clion (bundled with clang-tidy) to review nodejs cpp code :)

I use MSVS2017/9 which also has builtin clang-tidy integration (e.g. #23793). I also use Resharper (also by JetBrains) and sometimes I use CLion. I agree that proper use of better tools should allow us to improve our codebase.

We have had a bit of discussion around clang-tidy - https://github.com/nodejs/node/search?q=clang-tidy&type=Issues
I think in general the mood is positive, but IIUC there is no automatic consensus around it ¯\(ツ)/¯

I'd be happy to hear other contributors' opinions.

bnoordhuis
bnoordhuis reviewed Feb 11, 2019
View reviewed changes
@gengjiawen
Copy link
Member Author

gengjiawen commented Feb 11, 2019

I found MSVS2017 is pretty lame in handling cpp tbh.

@gengjiawen gengjiawen force-pushed the clang-tidy-perf-value-param branch from fdd0d8e to fe34e44 Compare February 12, 2019 12:08
@gengjiawen
Copy link
Member Author

gengjiawen commented Feb 13, 2019

@addaleax Any thought on this clang-tidy rule ?

@addaleax
Copy link
Member

addaleax commented Feb 13, 2019

@gengjiawen I don’t know … if the compiler can optimize this away (and it should), then yeah, I don’t know and I don’t think I have much to add to what the others here have said.

I do like the changes to node_url.h and the cctest, and I’d definitely want to keep them.

@gengjiawen
Copy link
Member Author

gengjiawen commented Feb 13, 2019

Should we start an issue to discuss https://clang.llvm.org/extra/clang-tidy/checks/performance-unnecessary-value-param.html with node cpp team (I am not sure it exists) ?

Or just keep the change node_url.h and cctest, and revert others ?

Thoughts ? cc @refack @addaleax

@addaleax
Copy link
Member

addaleax commented Feb 13, 2019

Should we start an issue to discuss https://clang.llvm.org/extra/clang-tidy/checks/performance-unnecessary-value-param.html with node cpp team (I am not sure it exists) ?

I think this PR is that issue :)

I have no strong feelings either way.

@refack
Copy link
Contributor

refack commented Feb 14, 2019 •
edited
Loading

I'm +1 on the const std::string& changes (since std::string is mutable the const is a nice bonus).

I'm -0 on the changes that add std::moveing of smart pointers in constructors.
IIUC the elision of their redundant re-construction is creeping into the ISO standard as mandatory anyway.

I'm -1 on changes that put const std::shared_ptr<T>& arg_ in the signature of APIs that actually do take a copy of the pointer.

@gengjiawen
Copy link
Member Author

gengjiawen commented Feb 14, 2019

@refack I will try to do revert the const std::shared_ptr<T>& arg_ part tomorrow.

@gengjiawen gengjiawen force-pushed the clang-tidy-perf-value-param branch from fe34e44 to ec6bcaa Compare February 15, 2019 14:06
@gengjiawen
Copy link
Member Author

gengjiawen commented Feb 15, 2019

@refack @addaleax Reverted the const std::shared_ptr<T>& arg_ part.

@addaleax
Copy link
Member

addaleax commented Feb 15, 2019 •
edited
Loading

CI: https://ci.nodejs.org/job/node-test-pull-request/20799/ (:heavy_check_mark:)

@gengjiawen
Copy link
Member Author

gengjiawen commented Feb 19, 2019

@danbev Can you import this change ? Thanks.

@danbev
Copy link
Contributor

danbev commented Feb 19, 2019

Landed in 8375c70.

@danbev danbev closed this Feb 19, 2019
danbev pushed a commit that referenced this pull request Feb 19, 2019
@gengjiawen @danbev
src: apply clang-tidy rule performance-unnecessary-value-param
8375c70 
PR-URL: #26042
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
@gengjiawen
Copy link
Member Author

gengjiawen commented Feb 19, 2019

Thanks.

@gengjiawen gengjiawen deleted the clang-tidy-perf-value-param branch February 19, 2019 12:17
addaleax pushed a commit that referenced this pull request Feb 19, 2019
@gengjiawen @addaleax
src: apply clang-tidy rule performance-unnecessary-value-param
79376ce 
PR-URL: #26042
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
@refack refack mentioned this pull request Feb 25, 2019
Merged
2 tasks
@BridgeAR BridgeAR mentioned this pull request Feb 26, 2019
Merged
rvagg pushed a commit that referenced this pull request Feb 28, 2019
@gengjiawen @rvagg
src: apply clang-tidy rule performance-unnecessary-value-param
25ddbc9 
PR-URL: #26042
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bnoordhuis bnoordhuis bnoordhuis left review comments

@jasnell jasnell jasnell approved these changes

@addaleax addaleax addaleax approved these changes

+1 more reviewer

@refack refack refack left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@gengjiawen @refack @addaleax @danbev @bnoordhuis @jasnell @nodejs-github-bot