FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

src: apply clang-tidy rule performance-unnecessary-value-param by gengjiawen · Pull Request #26042 · nodejs/node · GitHub

/ node Public

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

Closed
gengjiawen wants to merge 1 commit into
nodejs:masterfrom
gengjiawen:clang-tidy-perf-value-param
ConversationCommits (1)ChecksFiles changed
Closed

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

Conversation

gengjiawen commented Feb 11, 2019

Copy link
Copy Markdown
Member

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 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 force-pushed the clang-tidy-perf-value-param branch from 9e5dae3 to fdd0d8e Compare February 11, 2019 10:49
refack reviewed Feb 11, 2019
View reviewed changes

refack left a comment •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

Comment thread src/inspector/main_thread_interface.cc Outdated
Comment thread src/inspector/main_thread_interface.cc Outdated

refack commented Feb 11, 2019 •
edited
Loading

Copy link
Copy Markdown
Contributor

@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 commented Feb 11, 2019

Copy link
Copy Markdown
Member Author

@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 commented Feb 11, 2019

Copy link
Copy Markdown
Contributor

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 reviewed Feb 11, 2019
View reviewed changes
Comment thread src/inspector/worker_inspector.cc Outdated

gengjiawen commented Feb 11, 2019

Copy link
Copy Markdown
Member Author

I found MSVS2017 is pretty lame in handling cpp tbh.

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

gengjiawen commented Feb 13, 2019

Copy link
Copy Markdown
Member Author

@addaleax Any thought on this clang-tidy rule ?

addaleax commented Feb 13, 2019

Copy link
Copy Markdown
Member

@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 commented Feb 13, 2019

Copy link
Copy Markdown
Member Author

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 commented Feb 13, 2019

Copy link
Copy Markdown
Member

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 commented Feb 14, 2019 •
edited
Loading

Copy link
Copy Markdown
Contributor

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 commented Feb 14, 2019

Copy link
Copy Markdown
Member Author

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

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

gengjiawen commented Feb 15, 2019

Copy link
Copy Markdown
Member Author

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

addaleax commented Feb 15, 2019 •
edited
Loading

Copy link
Copy Markdown
Member

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

gengjiawen commented Feb 19, 2019

Copy link
Copy Markdown
Member Author

@danbev Can you import this change ? Thanks.

danbev commented Feb 19, 2019

Copy link
Copy Markdown
Contributor

Landed in 8375c70.

danbev closed this Feb 19, 2019
danbev pushed a commit that referenced this pull request Feb 19, 2019
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 commented Feb 19, 2019

Copy link
Copy Markdown
Member Author

Thanks.

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
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 mentioned this pull request Feb 25, 2019
Merged
2 tasks
BridgeAR mentioned this pull request Feb 26, 2019
Merged
rvagg pushed a commit that referenced this pull request Feb 28, 2019
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>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

bnoordhuis bnoordhuis left review comments
jasnell jasnell approved these changes
addaleax addaleax approved these changes
+1 more reviewer
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

Back | FazBrowse Home | New Git URL