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.

Sign up for GitHub

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

build: update lint-cpp on Windows #18012

Closed
wants to merge 2 commits into from
Closed

build: update lint-cpp on Windows #18012

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
58c13eb
build: update lint-cpp on Windows
kfarnung Jan 6, 2018
2a56d41
[squash] Add comment to subroutine
kfarnung Jan 16, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 8 additions & 6 deletions vcbuild.bat
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,7 @@ if /i "%1"=="test-v8" set test_v8=1&set custom_v8_test=1&goto arg-ok
if /i "%1"=="test-v8-intl" set test_v8_intl=1&set custom_v8_test=1&goto arg-ok
if /i "%1"=="test-v8-benchmarks" set test_v8_benchmarks=1&set custom_v8_test=1&goto arg-ok
if /i "%1"=="test-v8-all" set test_v8=1&set test_v8_intl=1&set test_v8_benchmarks=1&set custom_v8_test=1&goto arg-ok
if /i "%1"=="lint-cpp" set lint_cpp=1&goto arg-ok
if /i "%1"=="lint-js" set lint_js=1&goto arg-ok
if /i "%1"=="jslint" set lint_js=1&echo Please use lint-js instead of jslint&goto arg-ok
if /i "%1"=="lint-js-ci" set lint_js_ci=1&goto arg-ok
Expand Down Expand Up @@ -480,22 +481,23 @@ call :run-python tools/cpplint.py %cppfilelist% > nul
goto exit

:add-to-list
echo %1 | findstr /c:"src\node_root_certs.h"
@rem Subroutine used to filter items from the cpplint file list
echo %1 | findstr /c:"src\node_root_certs.h" > nul 2>&1
if %errorlevel% equ 0 goto exit

echo %1 | findstr /r /c:"src\\tracing\\trace_event.h"
echo %1 | findstr /c:"src\tracing\trace_event.h" > nul 2>&1
if %errorlevel% equ 0 goto exit

echo %1 | findstr /r /c:"src\\tracing\\trace_event_common.h"
echo %1 | findstr /c:"src\tracing\trace_event_common.h" > nul 2>&1
if %errorlevel% equ 0 goto exit

echo %1 | findstr /r /c:"test\\addons\\[0-9].*_.*\.h"
echo %1 | findstr /r /c:"test\\addons\\[0-9].*_.*\.h" > nul 2>&1
if %errorlevel% equ 0 goto exit

echo %1 | findstr /r /c:"test\\addons\\[0-9].*_.*\.cc"
echo %1 | findstr /r /c:"test\\addons\\[0-9].*_.*\.cc" > nul 2>&1
Copy link
Member

Choose a reason for hiding this comment

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

AIUI if one of these files is found then vcbuild bails out, wouldn't it be helpful to see which one it was?

Copy link
Member

Choose a reason for hiding this comment

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

Not the whole vcbuild, only the add-to-list subroutine. I don't really have a strong opinion about the redirects here, stdout is already discarded when calling add-to-list, but it can perhaps help if there's an error in findstr. @kfarnung the output with echo on is the same, is this helpful for you?

Copy link
Contributor Author

@kfarnung kfarnung Jan 11, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

@gibfahn This section of code is called elsewhere in the script. Because of this it's treated as though it's a separate batch execution. The early exits here are more akin to early returns in other languages. This merely filters the list of files that are sent into the CPP linter.

@joaocgreis The output is actually to stdout from what I can see. If you test out findstr manually it outputs all successful matches and outputs nothing when it doesn't match. Since echo is being used to pipe into findstr I don't believe that echo on has any effect here.

The reason for the redirects is that I actually found these counter-intuitive. I always assumed it was listing the files it was scanning, not the ones that it was ignoring. If there's strong opposition it might be worth trying to update the output so that it indicates that these are the files being ignored, not the ones being scanned.

if %errorlevel% equ 0 goto exit

echo %1 | findstr /c:"test\\addons-napi\\common.h"
echo %1 | findstr /c:"test\addons-napi\common.h" > nul 2>&1
if %errorlevel% equ 0 goto exit

set "localcppfilelist=%localcppfilelist% %1"
Expand Down