tools: fix check-imports.py to match on word boundaries by richardlau · Pull Request #33268 · nodejs/node

richardlau · 2020-05-06T16:38:55Z

check-imports.py was missing some unused using statements as it
was not matching on word boundaries (e.g. MaybeLocal was considered
a use of Local). Fix that and add some unit tests (which required
the script to be renamed to drop the - so it could be imported into
the test script).

Refs: #29226

Marking as blocked as now the script is fixed it reveals the following unused imports:

-bash-4.2$ make lint-cpp
Running C++ linter...
File "src/node.cc" does not use "HandleScope"
File "src/node.cc" does not use "Maybe"
File "src/inspector_agent.cc" does not use "String"
File "src/inspector_agent.cc" does not use "Task"
File "src/node_contextify.cc" does not use "ArrayBuffer"
File "src/node_dtrace.cc" does not use "String"
File "src/heap_utils.cc" does not use "JSON"
File "src/node_util.cc" does not use "Function"
File "src/tcp_wrap.cc" does not use "HandleScope"
File "src/module_wrap.cc" does not use "Just"
File "src/module_wrap.cc" does not use "Maybe"
File "src/node_zlib.cc" does not use "Array"
File "src/node_zlib.cc" does not use "Uint32"
File "src/pipe_wrap.cc" does not use "HandleScope"
File "src/tls_wrap.cc" does not use "Maybe"
File "src/tty_wrap.cc" does not use "Function"
File "src/inspector_js_api.cc" does not use "Boolean"
File "src/node_perf.cc" does not use "Array"
File "src/node_perf.cc" does not use "Name"
File "src/node_perf.cc" does not use "Uint32Array"
File "src/node_http2.cc" does not use "Float64Array"
File "src/node_http2.cc" does not use "Uint32"
File "src/node_http2.cc" does not use "Uint32Array"
File "src/node_messaging.cc" does not use "Exception"
File "src/node_options.cc" does not use "String"
File "src/node_credentials.cc" does not use "Function"
File "src/node_errors.cc" does not use "Number"
File "src/node_native_module.cc" does not use "HandleScope"
File "src/node_native_module.cc" does not use "Maybe"
File "src/node_native_module.cc" does not use "Script"
File "src/node_process_methods.cc" does not use "Function"
File "src/node_process_methods.cc" does not use "Name"
File "src/node_process_object.cc" does not use "HandleScope"
File "src/node_process_object.cc" does not use "Just"
File "src/node_report.cc" does not use "Number"
File "src/node_report.cc" does not use "StackTrace"
File "src/node_report.cc" does not use "Value"
File "src/node_report_module.cc" does not use "Boolean"
File "src/node_report_module.cc" does not use "Function"
File "src/node_dir.cc" does not use "Function"
File "src/node_main_instance.cc" does not use "Object"
File "src/node_native_module_env.cc" does not use "Maybe"
make: *** [tools/.cpplintstamp] Error 1
-bash-4.2$

#33261 will fix the ones in src/node.cc. Anyone have opinions on the best way to fix the remainder commit-wise? Should we fix them all in one commit, or try to group them up (e.g. src/node_report.cc and src/node_report_module.cc are the report subsystem)? @nodejs/releasers would it be easier to backport if they were all fixed up under one pull request or under several smaller pull requests?

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
commit message follows commit guidelines

nodejs-github-bot · 2020-05-06T16:49:22Z

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

richardlau · 2020-05-06T17:51:27Z

I've no idea why but the GitHub Actions run is only showing one file (src/node_process_object.cc):
https://github.com/nodejs/node/pull/33268/checks?check_run_id=650248933

Running C++ linter...
File "src/node_process_object.cc" does not use "HandleScope"
File "src/node_process_object.cc" does not use "Just"
Makefile:1329: recipe for target 'tools/.cpplintstamp' failed
make: *** [tools/.cpplintstamp] Error 1

The CI run (https://ci.nodejs.org/job/node-test-linter/34578/console) looks to agree with the list in the OP.

BridgeAR · 2020-05-23T17:36:26Z

@nodejs/automation @nodejs/python PTAL

bnoordhuis

LGTM, thanks. The fix-ups should be done in the same commit, or in a commit preceding the changes to the python script, that way make test keeps passing (important for git bisect.)

test/tools/test_checkimports.py

nodejs-github-bot · 2020-05-27T16:41:00Z

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

richardlau · 2020-05-27T16:46:43Z

Anyone have opinions on the best way to fix the remainder commit-wise? Should we fix them all in one commit, or try to group them up (e.g. src/node_report.cc and src/node_report_module.cc are the report subsystem)? @nodejs/releasers would it be easier to backport if they were all fixed up under one pull request or under several smaller pull requests?

In the absence of anyone advocating the fix ups be done in smaller commits and since @danbev applied the code fix ups in #33565 I've cherry-picked his commit over here and reordered so it comes first.

BridgeAR

RSLGTM

nodejs-github-bot · 2020-05-27T23:46:37Z

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

cclauss

RSLGTM

nodejs-github-bot · 2020-05-28T04:25:18Z

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

nodejs-github-bot · 2020-05-28T09:22:53Z

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

nodejs-github-bot · 2020-05-28T09:50:02Z

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

nodejs-github-bot · 2020-05-28T10:31:18Z

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

nodejs-github-bot · 2020-05-28T10:54:03Z

CI: https://ci.nodejs.org/job/node-test-pull-request/31599/ (✔️)

This commit removes the unused using declarations reported by lint-cpp. PR-URL: nodejs#33268 Refs: nodejs#29226 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Christian Clauss <cclauss@me.com> Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com> Reviewed-By: Beth Griggs <Bethany.Griggs@uk.ibm.com>

`check-imports.py` was missing some unused `using` statements as it was not matching on word boundaries (e.g. `MaybeLocal` was considered a use of `Local`). Fix that and add some unit tests (which required the script to be renamed to drop the `-` so it could be imported into the test script). PR-URL: nodejs#33268 Refs: nodejs#29226 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Christian Clauss <cclauss@me.com> Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com> Reviewed-By: Beth Griggs <Bethany.Griggs@uk.ibm.com>

richardlau · 2020-05-28T14:06:30Z

Landed in 785842a...05db682.

This commit removes the unused using declarations reported by lint-cpp. PR-URL: #33268 Refs: #29226 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Christian Clauss <cclauss@me.com> Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com> Reviewed-By: Beth Griggs <Bethany.Griggs@uk.ibm.com>

`check-imports.py` was missing some unused `using` statements as it was not matching on word boundaries (e.g. `MaybeLocal` was considered a use of `Local`). Fix that and add some unit tests (which required the script to be renamed to drop the `-` so it could be imported into the test script). PR-URL: #33268 Refs: #29226 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Christian Clauss <cclauss@me.com> Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com> Reviewed-By: Beth Griggs <Bethany.Griggs@uk.ibm.com>

This commit removes the unused using declarations reported by lint-cpp. PR-URL: #33268 Refs: #29226 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Christian Clauss <cclauss@me.com> Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com> Reviewed-By: Beth Griggs <Bethany.Griggs@uk.ibm.com>

`check-imports.py` was missing some unused `using` statements as it was not matching on word boundaries (e.g. `MaybeLocal` was considered a use of `Local`). Fix that and add some unit tests (which required the script to be renamed to drop the `-` so it could be imported into the test script). PR-URL: #33268 Refs: #29226 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Christian Clauss <cclauss@me.com> Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com> Reviewed-By: Beth Griggs <Bethany.Griggs@uk.ibm.com>

This commit removes the unused using declarations reported by lint-cpp. PR-URL: #33268 Refs: #29226 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Christian Clauss <cclauss@me.com> Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com> Reviewed-By: Beth Griggs <Bethany.Griggs@uk.ibm.com>

richardlau added the blocked PRs that are blocked by other issues or PRs. label May 6, 2020

nodejs-github-bot added build Issues and PRs related to build files or the CI. tools Issues and PRs related to the tools directory. labels May 6, 2020

richardlau mentioned this pull request May 6, 2020

src: remove unused using declarations in node.cc #33261

Closed

2 tasks

richardlau mentioned this pull request May 26, 2020

tools: update check-imports using function #33565

Closed

2 tasks

bnoordhuis approved these changes May 26, 2020

View reviewed changes

test/tools/test_checkimports.py Outdated Show resolved Hide resolved

richardlau force-pushed the cpplint branch 2 times, most recently from dc5e574 to a7737c0 Compare May 27, 2020 16:34

richardlau removed the blocked PRs that are blocked by other issues or PRs. label May 27, 2020

BridgeAR approved these changes May 27, 2020

View reviewed changes

BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label May 27, 2020

cclauss approved these changes May 28, 2020

View reviewed changes

danbev approved these changes May 28, 2020

View reviewed changes

riyadhalnur approved these changes May 28, 2020

View reviewed changes

BethGriggs approved these changes May 28, 2020

View reviewed changes

danbev and others added 2 commits May 28, 2020 10:02

richardlau force-pushed the cpplint branch from a7737c0 to 05db682 Compare May 28, 2020 14:05

richardlau merged commit 05db682 into nodejs:master May 28, 2020

richardlau deleted the cpplint branch May 28, 2020 14:08

codebytere mentioned this pull request Jun 28, 2020

v14.5.0 proposal #34093

Merged

codebytere mentioned this pull request Jul 13, 2020

v12.18.3 proposal #34343

Merged

Uh oh!

Conversation

richardlau commented May 6, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

nodejs-github-bot commented May 6, 2020

Uh oh!

richardlau commented May 6, 2020

Uh oh!

BridgeAR commented May 23, 2020

Uh oh!

bnoordhuis left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

nodejs-github-bot commented May 27, 2020

Uh oh!

richardlau commented May 27, 2020

Uh oh!

BridgeAR left a comment

Choose a reason for hiding this comment

Uh oh!

nodejs-github-bot commented May 27, 2020

Uh oh!

cclauss left a comment

Choose a reason for hiding this comment

Uh oh!

nodejs-github-bot commented May 28, 2020

Uh oh!

nodejs-github-bot commented May 28, 2020

Uh oh!

nodejs-github-bot commented May 28, 2020

Uh oh!

nodejs-github-bot commented May 28, 2020

Uh oh!

nodejs-github-bot commented May 28, 2020 • edited by richardlau Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

richardlau commented May 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

richardlau commented May 6, 2020 •

edited

Loading

nodejs-github-bot commented May 28, 2020 •

edited by richardlau

Loading

richardlau commented May 28, 2020 •

edited

Loading