tools: add clang-tidy rule in src

[gengjiawen]

In short

• make cpp check rules explicit and get checked by CI
• select the rules most collaborators are comfortable with

Todo

Tools to ensure the rules

@refack told me some rules are checked.
Basically the rules should check most operating systems because we have platform-specific code such as using macro.
And should keep clang-tidy in same version when possible.
I have some thought on docker get involved in this. Hopefully I can put my thought
in node\build group this weekends.

Google Cpp Guideline

Related to #24315.
clang-tidy can handle some google cpp guideline rules.
I don't know should we

• use clang-tidy to ensure google-related rule
• should we add all google- or just some of them.
  see https://clang.llvm.org/extra/clang-tidy/

Rules need to be select.

The rules now included are already applied or in-review.

How to deal with false positive and make exceptions

.

cc @addaleax @bnoordhuis @refack @joyeecheung @jasnell @richardlau @danbev @Trott @BridgeAR

Checklist

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

[refack]

@refack told me some rules are checked.

What I was referring to is V8's rule set.
But with @gengjiawen's help I've made https://ci.nodejs.org/job/node-clang-tidy/ to run for our code.

CI for this: https://ci.nodejs.org/job/node-clang-tidy/5/console
Result in https://gist.github.com/refack/ce2d10f8364a539c5da3314a777bee3e

[BridgeAR]

What's the status here?

[gengjiawen]

What's the status here?

Need to change some rules in my side. Not know the progress in CI setup ? @refack

[refack]

Let's see https://ci.nodejs.org/job/node-clang-tidy/6/
[Update:] 70844 warnings and 37 errors generated.

BTW: Apparently there a more comprehensive wrapper around LLVM's LibTooling - clang-check. Locally it gives me less false-positives.

[gengjiawen]

I update the rules to disable performance-unnecessary-value-param since it's partial applied and enable modernize-deprecated-headers since it's fixed.

Also not quite sure modernize-use-equals-default are used since it's not introduced.
@refack Rerun node-clang-tidy ?

[refack]

It's in the queue and will soon show up as https://ci.nodejs.org/job/node-clang-tidy/7/

[gengjiawen]

Rebase after the four PRs got merged.
#27280
#27279
#27264
#27263

[refack]

Rebased over #27264
https://ci.nodejs.org/job/node-clang-tidy/10/console

Filtered results https://gist.github.com/refack/8490362a2dd0dabb444a9e9f3ea1bd98

[gengjiawen]

Rebased over #27264
ci.nodejs.org/job/node-clang-tidy/10/console

Filtered results gist.github.com/refack/8490362a2dd0dabb444a9e9f3ea1bd98

I rebased and some fix for the left. Can you trigger this task again ?

[gengjiawen]

Revert node-url change for https://travis-ci.com/nodejs/node/jobs/196150407#L3090

../src/node_url.cc: In destructor ‘node::url::{anonymous}::URLHost::~URLHost()’:
../src/node_url.cc:123:19: error: use of deleted function ‘node::url::{anonymous}::URLHost::Value::~Value()’
 URLHost::~URLHost() {
                   ^
../src/node_url.cc:82:5: note: ‘node::url::{anonymous}::URLHost::Value::~Value()’ is implicitly deleted because the default definition would be ill-formed:
     ~Value() = default;

[bnoordhuis]

Nice, LGTM!

[gengjiawen]

@refack Need to retrigger the Jenkins task.

[gengjiawen]

@refack ping

[BridgeAR]

@gengjiawen is there something you have to wait upon to land this besides rebasing?

[gengjiawen]

@gengjiawen is there something you have to wait upon to land this besides rebasing?

This can land. But @refack works on a Jenkins job can make sure the rules checks on CI.
I guess we can land it, save CI job for later.

[richardlau]

@gengjiawen is there something you have to wait upon to land this besides rebasing?

This can land. But @refack works on a Jenkins job can make sure the rules checks on CI.
I guess we can land it, save CI job for later.

I ran the job with default parameters and it failed: https://ci.nodejs.org/job/node-clang-tidy/12/console
Given we don't currently run this on the CI maybe this is a candidate for running as a GitHub action?

[gengjiawen]

I ran the job with default parameters and it failed: ci.nodejs.org/job/node-clang-tidy/12/console
Given we don't currently run this on the CI maybe this is a candidate for running as a GitHub action?

Sure. But I am not familiar with related cpp tools. I am thinking landing this change first, thoughts ?

[richardlau]

I ran the job with default parameters and it failed: ci.nodejs.org/job/node-clang-tidy/12/console
Given we don't currently run this on the CI maybe this is a candidate for running as a GitHub action?

Sure. But I am not familiar with related cpp tools. I am thinking landing this change first, thoughts ?

I don't use clang nor clang-tidy so I have no opinion on the rules.

[gengjiawen]

Force push to resolve conflict.

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/28323/

[Trott]

Landed in a42dcbe