Skip to content

Conversation

@danbev
Copy link
Contributor

This commit removes the normal header file include if an internal one
is specified as per the CPP_STYLE_GUIDE.

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

This commit removes the normal header file include if an internal one is specified as per the CPP_STYLE_GUIDE.
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-botnodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. labels Jun 18, 2018
@danbev
Copy link
ContributorAuthor

@BridgeARBridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jun 18, 2018
@danbev
Copy link
ContributorAuthor

node-test-commit-windows-fanned failure looks unrelated

console output:

not ok 491 parallel/test-worker-memory --- duration_ms: 1.413 severity: fail exitcode: 1 stack: |- Mismatched <anonymous> function calls. Expected exactly 1, actual 0. at Object.exports.mustCall (c:\workspace\node-test-binary-windows\test\common\index.js:428:10) at run (c:\workspace\node-test-binary-windows\test\parallel\test-worker-memory.js:21:28) at Worker.worker.on.common.mustCall (c:\workspace\node-test-binary-windows\test\parallel\test-worker-memory.js:22:5) at Worker.<anonymous> (c:\workspace\node-test-binary-windows\test\common\index.js:468:15) at Worker.emit (events.js:182:13) at Worker.[kOnExit] (internal/worker.js:270:10) at Worker.(anonymous function).onexit (internal/worker.js:226:51) ...

Copy link
Member

@bnoordhuisbnoordhuis left a comment

Choose a reason for hiding this comment

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

Might be nice as an enhancement to have cpplint.py or check-imports.sh enforce this.

@danbev
Copy link
ContributorAuthor

Might be nice as an enhancement to have cpplint.py or check-imports.sh enforce this.

Yeah, I what would make sense. I'll take a look but might not have time this week by the looks of things.

@ryzokuken
Copy link
Contributor

@danbev if it's okay, I guess this could be landed and I could help out with the linting in a separate PR.

@danbev
Copy link
ContributorAuthor

if it's okay, I guess this could be landed and I could help out with the linting in a separate PR.

That would be great, thanks!

@danbev
Copy link
ContributorAuthor

Landed in a71d5fc.

@danbevdanbev closed this Jun 20, 2018
@danbevdanbev deleted the remove-header-when-internal-is-used branch June 20, 2018 03:48
danbev added a commit that referenced this pull request Jun 20, 2018
This commit removes the normal header file include if an internal one is specified as per the CPP_STYLE_GUIDE. PR-URL: #21381 Reviewed-By: Matheus Marchini <[email protected]> Reviewed-By: Gus Caplan <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Tiancheng "Timothy" Gu <[email protected]> Reviewed-By: Ujjwal Sharma <[email protected]>
@targostargos added backport-requested-v10.x and removed author ready PRs that have at least one approval, no pending requests for changes, and a CI started. labels Jun 20, 2018
@targos
Copy link
Member

targos commented Jun 20, 2018

Should land cleanly on v10.x-staging after #21105 is backported

targos pushed a commit that referenced this pull request Jul 14, 2018
This commit removes the normal header file include if an internal one is specified as per the CPP_STYLE_GUIDE. PR-URL: #21381 Reviewed-By: Matheus Marchini <[email protected]> Reviewed-By: Gus Caplan <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Tiancheng "Timothy" Gu <[email protected]> Reviewed-By: Ujjwal Sharma <[email protected]>
@targostargos mentioned this pull request Jul 17, 2018
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

c++Issues and PRs that require attention from people who are familiar with C++.lib / srcIssues and PRs related to general changes in the lib or src directory.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

11 participants

@danbev@nodejs-github-bot@ryzokuken@targos@bnoordhuis@lpinca@TimothyGu@cjihrig@mmarchini@devsnek@BridgeAR