From 4a20cc937f6a2b830a2752e35da7e389bbc4e006 Mon Sep 17 00:00:00 2001 From: Daniel Bevenius Date: Tue, 26 May 2020 08:48:17 +0200 Subject: [PATCH 1/2] src: remove unused using declarations This commit removes the unused using declarations reported by lint-cpp. PR-URL: https://github.com/nodejs/node/pull/33268 Refs: https://github.com/nodejs/node/issues/29226 Reviewed-By: Ben Noordhuis Reviewed-By: Ruben Bridgewater Reviewed-By: Christian Clauss Reviewed-By: Daniel Bevenius Reviewed-By: Beth Griggs --- src/async_wrap.cc | 1 - src/heap_utils.cc | 1 - src/inspector_agent.cc | 2 -- src/inspector_js_api.cc | 1 - src/module_wrap.cc | 2 -- src/node_contextify.cc | 1 - src/node_credentials.cc | 1 - src/node_dir.cc | 1 - src/node_dtrace.cc | 1 - src/node_errors.cc | 1 - src/node_http2.cc | 3 --- src/node_main_instance.cc | 1 - src/node_messaging.cc | 1 - src/node_native_module.cc | 3 --- src/node_native_module_env.cc | 1 - src/node_options.cc | 1 - src/node_perf.cc | 3 --- src/node_process_methods.cc | 2 -- src/node_process_object.cc | 2 -- src/node_report.cc | 3 --- src/node_report_module.cc | 2 -- src/node_util.cc | 1 - src/node_zlib.cc | 2 -- src/pipe_wrap.cc | 1 - src/tcp_wrap.cc | 1 - src/tls_wrap.cc | 1 - src/tty_wrap.cc | 1 - 27 files changed, 41 deletions(-) diff --git a/src/async_wrap.cc b/src/async_wrap.cc index 46a6ea6eebe85f..13c2c3ca5a5ec3 100644 --- a/src/async_wrap.cc +++ b/src/async_wrap.cc @@ -39,7 +39,6 @@ using v8::HandleScope; using v8::Integer; using v8::Isolate; using v8::Local; -using v8::Maybe; using v8::MaybeLocal; using v8::Name; using v8::Number; diff --git a/src/heap_utils.cc b/src/heap_utils.cc index 2e979e49e87922..386bf61e4eca00 100644 --- a/src/heap_utils.cc +++ b/src/heap_utils.cc @@ -15,7 +15,6 @@ using v8::Global; using v8::HandleScope; using v8::HeapSnapshot; using v8::Isolate; -using v8::JSON; using v8::Local; using v8::MaybeLocal; using v8::Number; diff --git a/src/inspector_agent.cc b/src/inspector_agent.cc index 74b7fc13cde4c0..f128a197ca9726 100644 --- a/src/inspector_agent.cc +++ b/src/inspector_agent.cc @@ -45,8 +45,6 @@ using v8::Isolate; using v8::Local; using v8::Message; using v8::Object; -using v8::String; -using v8::Task; using v8::Value; using v8_inspector::StringBuffer; diff --git a/src/inspector_js_api.cc b/src/inspector_js_api.cc index 5c9ad5e946424b..c318f78646634d 100644 --- a/src/inspector_js_api.cc +++ b/src/inspector_js_api.cc @@ -12,7 +12,6 @@ namespace node { namespace inspector { namespace { -using v8::Boolean; using v8::Context; using v8::Function; using v8::FunctionCallbackInfo; diff --git a/src/module_wrap.cc b/src/module_wrap.cc index 59ef9daf727ed5..ab8dbc9cbf7fa5 100644 --- a/src/module_wrap.cc +++ b/src/module_wrap.cc @@ -33,9 +33,7 @@ using v8::HandleScope; using v8::Integer; using v8::IntegrityLevel; using v8::Isolate; -using v8::Just; using v8::Local; -using v8::Maybe; using v8::MaybeLocal; using v8::Module; using v8::Number; diff --git a/src/node_contextify.cc b/src/node_contextify.cc index 99adccbcef9d64..69663afe423f58 100644 --- a/src/node_contextify.cc +++ b/src/node_contextify.cc @@ -36,7 +36,6 @@ namespace contextify { using errors::TryCatchScope; using v8::Array; -using v8::ArrayBuffer; using v8::ArrayBufferView; using v8::Boolean; using v8::Context; diff --git a/src/node_credentials.cc b/src/node_credentials.cc index ad0e1dbb9bb68e..d7be988d978b8a 100644 --- a/src/node_credentials.cc +++ b/src/node_credentials.cc @@ -15,7 +15,6 @@ namespace node { using v8::Array; using v8::Context; -using v8::Function; using v8::FunctionCallbackInfo; using v8::HandleScope; using v8::Isolate; diff --git a/src/node_dir.cc b/src/node_dir.cc index cb32fa0a5c6629..7d1e1b1d7d6609 100644 --- a/src/node_dir.cc +++ b/src/node_dir.cc @@ -28,7 +28,6 @@ using fs::GetReqWrap; using v8::Array; using v8::Context; -using v8::Function; using v8::FunctionCallbackInfo; using v8::FunctionTemplate; using v8::HandleScope; diff --git a/src/node_dtrace.cc b/src/node_dtrace.cc index fc58734c59ae15..3c407f3447f171 100644 --- a/src/node_dtrace.cc +++ b/src/node_dtrace.cc @@ -57,7 +57,6 @@ using v8::HandleScope; using v8::Isolate; using v8::Local; using v8::Object; -using v8::String; using v8::Value; #define SLURP_STRING(obj, member, valp) \ diff --git a/src/node_errors.cc b/src/node_errors.cc index 22bc4d994b8859..4f8b003d8da225 100644 --- a/src/node_errors.cc +++ b/src/node_errors.cc @@ -25,7 +25,6 @@ using v8::Local; using v8::Maybe; using v8::MaybeLocal; using v8::Message; -using v8::Number; using v8::Object; using v8::ScriptOrigin; using v8::StackFrame; diff --git a/src/node_http2.cc b/src/node_http2.cc index 25f353bb1477e0..067e73b043b3d6 100644 --- a/src/node_http2.cc +++ b/src/node_http2.cc @@ -22,7 +22,6 @@ using v8::ArrayBufferView; using v8::Boolean; using v8::Context; using v8::EscapableHandleScope; -using v8::Float64Array; using v8::Function; using v8::FunctionCallbackInfo; using v8::FunctionTemplate; @@ -35,8 +34,6 @@ using v8::Number; using v8::Object; using v8::ObjectTemplate; using v8::String; -using v8::Uint32; -using v8::Uint32Array; using v8::Uint8Array; using v8::Undefined; using v8::Value; diff --git a/src/node_main_instance.cc b/src/node_main_instance.cc index e9cef5fc002284..f638e26dba5a04 100644 --- a/src/node_main_instance.cc +++ b/src/node_main_instance.cc @@ -20,7 +20,6 @@ using v8::HandleScope; using v8::Isolate; using v8::Local; using v8::Locker; -using v8::Object; using v8::SealHandleScope; NodeMainInstance::NodeMainInstance(Isolate* isolate, diff --git a/src/node_messaging.cc b/src/node_messaging.cc index 6081a523cd4c52..ffe29a5dea8c8a 100644 --- a/src/node_messaging.cc +++ b/src/node_messaging.cc @@ -16,7 +16,6 @@ using v8::BackingStore; using v8::CompiledWasmModule; using v8::Context; using v8::EscapableHandleScope; -using v8::Exception; using v8::Function; using v8::FunctionCallbackInfo; using v8::FunctionTemplate; diff --git a/src/node_native_module.cc b/src/node_native_module.cc index a57355418bea06..a7675d00d89cd3 100644 --- a/src/node_native_module.cc +++ b/src/node_native_module.cc @@ -7,14 +7,11 @@ namespace native_module { using v8::Context; using v8::EscapableHandleScope; using v8::Function; -using v8::HandleScope; using v8::Integer; using v8::Isolate; using v8::Local; -using v8::Maybe; using v8::MaybeLocal; using v8::Object; -using v8::Script; using v8::ScriptCompiler; using v8::ScriptOrigin; using v8::String; diff --git a/src/node_native_module_env.cc b/src/node_native_module_env.cc index be647b01c640b0..ae8d349541b912 100644 --- a/src/node_native_module_env.cc +++ b/src/node_native_module_env.cc @@ -11,7 +11,6 @@ using v8::FunctionCallbackInfo; using v8::IntegrityLevel; using v8::Isolate; using v8::Local; -using v8::Maybe; using v8::MaybeLocal; using v8::Name; using v8::None; diff --git a/src/node_options.cc b/src/node_options.cc index 875c2e73001dc9..a84420e3c5d375 100644 --- a/src/node_options.cc +++ b/src/node_options.cc @@ -18,7 +18,6 @@ using v8::Local; using v8::Map; using v8::Number; using v8::Object; -using v8::String; using v8::Undefined; using v8::Value; diff --git a/src/node_perf.cc b/src/node_perf.cc index fb3b2c43acdd9f..4ed1c956c9e52e 100644 --- a/src/node_perf.cc +++ b/src/node_perf.cc @@ -11,7 +11,6 @@ namespace node { namespace performance { -using v8::Array; using v8::Context; using v8::DontDelete; using v8::Function; @@ -25,14 +24,12 @@ using v8::Isolate; using v8::Local; using v8::Map; using v8::MaybeLocal; -using v8::Name; using v8::NewStringType; using v8::Number; using v8::Object; using v8::PropertyAttribute; using v8::ReadOnly; using v8::String; -using v8::Uint32Array; using v8::Value; // Microseconds in a millisecond, as a float. diff --git a/src/node_process_methods.cc b/src/node_process_methods.cc index 88f4c1cfbd0249..6013dbb86b72fc 100644 --- a/src/node_process_methods.cc +++ b/src/node_process_methods.cc @@ -36,13 +36,11 @@ using v8::ArrayBuffer; using v8::BigUint64Array; using v8::Context; using v8::Float64Array; -using v8::Function; using v8::FunctionCallbackInfo; using v8::HeapStatistics; using v8::Integer; using v8::Isolate; using v8::Local; -using v8::Name; using v8::NewStringType; using v8::Number; using v8::Object; diff --git a/src/node_process_object.cc b/src/node_process_object.cc index ca17da9583efc8..5bec65805a1024 100644 --- a/src/node_process_object.cc +++ b/src/node_process_object.cc @@ -15,10 +15,8 @@ using v8::EscapableHandleScope; using v8::Function; using v8::FunctionCallbackInfo; using v8::FunctionTemplate; -using v8::HandleScope; using v8::Integer; using v8::Isolate; -using v8::Just; using v8::Local; using v8::MaybeLocal; using v8::Name; diff --git a/src/node_report.cc b/src/node_report.cc index 98da24c9567a28..62b3d7abd1057b 100644 --- a/src/node_report.cc +++ b/src/node_report.cc @@ -41,11 +41,8 @@ using v8::HeapSpaceStatistics; using v8::HeapStatistics; using v8::Isolate; using v8::Local; -using v8::Number; -using v8::StackTrace; using v8::String; using v8::V8; -using v8::Value; namespace per_process = node::per_process; diff --git a/src/node_report_module.cc b/src/node_report_module.cc index 700dd88aba645e..5afc0cfe104fe6 100644 --- a/src/node_report_module.cc +++ b/src/node_report_module.cc @@ -18,9 +18,7 @@ namespace report { using node::Environment; using node::Mutex; using node::Utf8Value; -using v8::Boolean; using v8::Context; -using v8::Function; using v8::FunctionCallbackInfo; using v8::HandleScope; using v8::Isolate; diff --git a/src/node_util.cc b/src/node_util.cc index db9b8ec8d65f51..ec3f8e1fe7deaf 100644 --- a/src/node_util.cc +++ b/src/node_util.cc @@ -10,7 +10,6 @@ using v8::Array; using v8::ArrayBufferView; using v8::Boolean; using v8::Context; -using v8::Function; using v8::FunctionCallbackInfo; using v8::FunctionTemplate; using v8::Global; diff --git a/src/node_zlib.cc b/src/node_zlib.cc index 83698bd5192e4b..85e87327c07797 100644 --- a/src/node_zlib.cc +++ b/src/node_zlib.cc @@ -43,7 +43,6 @@ namespace node { -using v8::Array; using v8::ArrayBuffer; using v8::Context; using v8::Function; @@ -56,7 +55,6 @@ using v8::Integer; using v8::Local; using v8::Object; using v8::String; -using v8::Uint32; using v8::Uint32Array; using v8::Value; diff --git a/src/pipe_wrap.cc b/src/pipe_wrap.cc index c4a5b7cd62e1b4..9e6831b2ed3b04 100644 --- a/src/pipe_wrap.cc +++ b/src/pipe_wrap.cc @@ -39,7 +39,6 @@ using v8::EscapableHandleScope; using v8::Function; using v8::FunctionCallbackInfo; using v8::FunctionTemplate; -using v8::HandleScope; using v8::Int32; using v8::Isolate; using v8::Local; diff --git a/src/tcp_wrap.cc b/src/tcp_wrap.cc index 619c9ef6196373..ef1b80939e91f9 100644 --- a/src/tcp_wrap.cc +++ b/src/tcp_wrap.cc @@ -42,7 +42,6 @@ using v8::EscapableHandleScope; using v8::Function; using v8::FunctionCallbackInfo; using v8::FunctionTemplate; -using v8::HandleScope; using v8::Int32; using v8::Integer; using v8::Local; diff --git a/src/tls_wrap.cc b/src/tls_wrap.cc index 0c2c2bbc014cc3..8fa1bd2eea0116 100644 --- a/src/tls_wrap.cc +++ b/src/tls_wrap.cc @@ -47,7 +47,6 @@ using v8::HandleScope; using v8::Integer; using v8::Isolate; using v8::Local; -using v8::Maybe; using v8::MaybeLocal; using v8::Object; using v8::PropertyAttribute; diff --git a/src/tty_wrap.cc b/src/tty_wrap.cc index 8536fae3ed7383..401c2513dbc628 100644 --- a/src/tty_wrap.cc +++ b/src/tty_wrap.cc @@ -32,7 +32,6 @@ namespace node { using v8::Array; using v8::Context; -using v8::Function; using v8::FunctionCallbackInfo; using v8::FunctionTemplate; using v8::Integer; From 05db68245ba63bc908d09c1cc64098ce97fa9d1f Mon Sep 17 00:00:00 2001 From: Richard Lau Date: Wed, 6 May 2020 11:20:14 -0400 Subject: [PATCH 2/2] tools: fix check-imports.py to match on word boundaries `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: https://github.com/nodejs/node/pull/33268 Refs: https://github.com/nodejs/node/issues/29226 Reviewed-By: Ben Noordhuis Reviewed-By: Ruben Bridgewater Reviewed-By: Christian Clauss Reviewed-By: Daniel Bevenius Reviewed-By: Beth Griggs --- Makefile | 4 +- test/fixtures/tools/checkimports/invalid.cc | 7 ++ test/fixtures/tools/checkimports/maybe.cc | 7 ++ test/fixtures/tools/checkimports/unsorted.cc | 6 ++ test/fixtures/tools/checkimports/unused.cc | 5 ++ test/fixtures/tools/checkimports/valid.cc | 6 ++ test/tools/test_checkimports.py | 77 ++++++++++++++++++++ tools/{check-imports.py => checkimports.py} | 6 +- 8 files changed, 114 insertions(+), 4 deletions(-) create mode 100644 test/fixtures/tools/checkimports/invalid.cc create mode 100644 test/fixtures/tools/checkimports/maybe.cc create mode 100644 test/fixtures/tools/checkimports/unsorted.cc create mode 100644 test/fixtures/tools/checkimports/unused.cc create mode 100644 test/fixtures/tools/checkimports/valid.cc create mode 100644 test/tools/test_checkimports.py rename tools/{check-imports.py => checkimports.py} (84%) diff --git a/Makefile b/Makefile index 7a6e3d5c32d161..519e920685177f 100644 --- a/Makefile +++ b/Makefile @@ -1328,13 +1328,13 @@ else CPPLINT_QUIET = --quiet endif .PHONY: lint-cpp -# Lints the C++ code with cpplint.py and check-imports.py. +# Lints the C++ code with cpplint.py and checkimports.py. lint-cpp: tools/.cpplintstamp tools/.cpplintstamp: $(LINT_CPP_FILES) @echo "Running C++ linter..." @$(PYTHON) tools/cpplint.py $(CPPLINT_QUIET) $? - @$(PYTHON) tools/check-imports.py + @$(PYTHON) tools/checkimports.py @touch $@ .PHONY: lint-addon-docs diff --git a/test/fixtures/tools/checkimports/invalid.cc b/test/fixtures/tools/checkimports/invalid.cc new file mode 100644 index 00000000000000..deace1287f11bb --- /dev/null +++ b/test/fixtures/tools/checkimports/invalid.cc @@ -0,0 +1,7 @@ +using v8::MaybeLocal; +using v8::Array; +using v8::Local; + +MaybeLocal CreateObject() const { + return MaybeLocal(); +} diff --git a/test/fixtures/tools/checkimports/maybe.cc b/test/fixtures/tools/checkimports/maybe.cc new file mode 100644 index 00000000000000..e96899be775ae7 --- /dev/null +++ b/test/fixtures/tools/checkimports/maybe.cc @@ -0,0 +1,7 @@ +using v8::Array; +using v8::Local; +using v8::MaybeLocal; + +MaybeLocal CreateObject() const { + return MaybeLocal(); +} diff --git a/test/fixtures/tools/checkimports/unsorted.cc b/test/fixtures/tools/checkimports/unsorted.cc new file mode 100644 index 00000000000000..0e6b540dccc466 --- /dev/null +++ b/test/fixtures/tools/checkimports/unsorted.cc @@ -0,0 +1,6 @@ +using v8::MaybeLocal; +using v8::Array; + +MaybeLocal CreateObject() const { + return MaybeLocal(); +} diff --git a/test/fixtures/tools/checkimports/unused.cc b/test/fixtures/tools/checkimports/unused.cc new file mode 100644 index 00000000000000..231f5549e2900c --- /dev/null +++ b/test/fixtures/tools/checkimports/unused.cc @@ -0,0 +1,5 @@ +using v8::Context; + +static void MyMethod(void) { + return; +} diff --git a/test/fixtures/tools/checkimports/valid.cc b/test/fixtures/tools/checkimports/valid.cc new file mode 100644 index 00000000000000..7e52968883ad8b --- /dev/null +++ b/test/fixtures/tools/checkimports/valid.cc @@ -0,0 +1,6 @@ +using v8::Array; +using v8::MaybeLocal; + +MaybeLocal CreateObject() const { + return MaybeLocal(); +} diff --git a/test/tools/test_checkimports.py b/test/tools/test_checkimports.py new file mode 100644 index 00000000000000..6e8e17cc3f9c23 --- /dev/null +++ b/test/tools/test_checkimports.py @@ -0,0 +1,77 @@ +import unittest +import sys +from contextlib import contextmanager +from os import path +sys.path.append(path.abspath(path.join(path.dirname(__file__), + '..', '..', 'tools'))) +try: + from StringIO import StringIO +except ImportError: + from io import StringIO + +from checkimports import is_valid + +@contextmanager +def captured_output(): + tmp_out, tmp_err = StringIO(), StringIO() + old_out, old_err = sys.stdout, sys.stderr + try: + sys.stdout, sys.stderr = tmp_out, tmp_err + yield sys.stdout, sys.stderr + finally: + sys.stdout, sys.stderr = old_out, old_err + tmp_out.close() + tmp_err.close() + +class CheckImportsTest(unittest.TestCase): + fixturesDir = path.join(path.dirname(__file__), '..', '..', + 'test', 'fixtures', 'tools', 'checkimports') + + def test_unused_and_unsorted(self): + with captured_output() as (out, err): + self.assertEqual(is_valid(path.join(self.fixturesDir, 'invalid.cc')), + False) + output = out.getvalue() + self.assertIn('does not use "Local"', output); + self.assertIn('using statements aren\'t sorted in', output); + self.assertIn('Line 1: Actual: v8::MaybeLocal, Expected: v8::Array', + output); + self.assertIn('Line 2: Actual: v8::Array, Expected: v8::Local', + output); + self.assertIn('Line 3: Actual: v8::Local, Expected: v8::MaybeLocal', + output); + + def test_unused_complex(self): + with captured_output() as (out, err): + self.assertEqual(is_valid(path.join(self.fixturesDir, 'maybe.cc')), + False) + output = out.getvalue() + self.assertIn('does not use "Local"', output); + + def test_unused_simple(self): + with captured_output() as (out, err): + self.assertEqual(is_valid(path.join(self.fixturesDir, 'unused.cc')), + False) + output = out.getvalue() + self.assertIn('does not use "Context"', output); + + def test_unsorted(self): + with captured_output() as (out, err): + self.assertEqual(is_valid(path.join(self.fixturesDir, 'unsorted.cc')), + False) + output = out.getvalue() + self.assertIn('using statements aren\'t sorted in', output); + self.assertIn('Line 1: Actual: v8::MaybeLocal, Expected: v8::Array', + output); + self.assertIn('Line 2: Actual: v8::Array, Expected: v8::MaybeLocal', + output); + + def test_valid(self): + with captured_output() as (out, err): + self.assertEqual(is_valid(path.join(self.fixturesDir, 'valid.cc')), + True) + output = out.getvalue() + self.assertEqual(output, ''); + +if __name__ == '__main__': + unittest.main() diff --git a/tools/check-imports.py b/tools/checkimports.py similarity index 84% rename from tools/check-imports.py rename to tools/checkimports.py index 51b4e63aa03903..609a75f542748f 100755 --- a/tools/check-imports.py +++ b/tools/checkimports.py @@ -9,7 +9,7 @@ def do_exist(file_name, lines, imported): if not any(not re.match('using \w+::{0};'.format(imported), line) and - re.search(imported, line) for line in lines): + re.search('\\b{0}\\b'.format(imported), line) for line in lines): print('File "{0}" does not use "{1}"'.format(file_name, imported)) return False return True @@ -40,4 +40,6 @@ def is_valid(file_name): else: return valid -sys.exit(0 if all(map(is_valid, glob.iglob('src/*.cc'))) else 1) +if __name__ == '__main__': + files = glob.iglob(sys.argv[1] if len(sys.argv) > 1 else 'src/*.cc') + sys.exit(0 if all(map(is_valid, files)) else 1)