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

[gengjiawen]

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

[refack]

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]

@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]

@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]

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.

[gengjiawen]

I found MSVS2017 is pretty lame in handling cpp tbh.

[gengjiawen]

@addaleax Any thought on this clang-tidy rule ?

[addaleax]

@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]

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]

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]

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]

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

[gengjiawen]

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

[addaleax]

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

[gengjiawen]

@danbev Can you import this change ? Thanks.

[danbev]

Landed in 8375c70.

[gengjiawen]

Thanks.