Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

tools: fix check-imports.py to match on word boundaries #33268

Merged
merged 2 commits into from
May 28, 2020

Conversation

richardlau
Copy link
Member

@richardlau richardlau commented May 6, 2020

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

@richardlau richardlau added the blocked PRs that are blocked by other issues or PRs. label May 6, 2020
@nodejs-github-bot 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
@nodejs-github-bot
Copy link
Collaborator

@richardlau
Copy link
Member Author

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
Copy link
Member

@nodejs/automation @nodejs/python PTAL

Copy link
Member

@bnoordhuis bnoordhuis left a comment

Choose a reason for hiding this comment

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

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 Outdated Show resolved Hide resolved
@richardlau richardlau force-pushed the cpplint branch 2 times, most recently from dc5e574 to a7737c0 Compare May 27, 2020 16:34
@richardlau richardlau removed the blocked PRs that are blocked by other issues or PRs. label May 27, 2020
@nodejs-github-bot
Copy link
Collaborator

@richardlau
Copy link
Member Author

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.

Copy link
Member

@BridgeAR BridgeAR left a comment

Choose a reason for hiding this comment

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

RSLGTM

@BridgeAR 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
@nodejs-github-bot
Copy link
Collaborator

Copy link
Contributor

@cclauss cclauss left a comment

Choose a reason for hiding this comment

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

RSLGTM

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented May 28, 2020

danbev and others added 2 commits May 28, 2020 10:02
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
Copy link
Member Author

richardlau commented May 28, 2020

Landed in 785842a...05db682.

@richardlau richardlau merged commit 05db682 into nodejs:master May 28, 2020
@richardlau richardlau deleted the cpplint branch May 28, 2020 14:08
codebytere pushed a commit that referenced this pull request Jun 18, 2020
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>
codebytere pushed a commit that referenced this pull request Jun 18, 2020
`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>
@codebytere codebytere mentioned this pull request Jun 28, 2020
codebytere pushed a commit that referenced this pull request Jun 30, 2020
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>
codebytere pushed a commit that referenced this pull request Jun 30, 2020
`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>
codebytere pushed a commit that referenced this pull request Jul 8, 2020
`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>
codebytere pushed a commit that referenced this pull request Jul 8, 2020
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>
@codebytere codebytere mentioned this pull request Jul 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. build Issues and PRs related to build files or the CI. tools Issues and PRs related to the tools directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants