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

Jump to bottom

src: remove .h if -inl.h is already included #21381

Closed
wants to merge 1 commit into from
Closed

src: remove .h if -inl.h is already included #21381

wants to merge 1 commit into from

Conversation

danbev
Copy link
Contributor

@danbev danbev commented Jun 18, 2018

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

@danbev
src: remove .h if -inl.h is already included
415aa2e 
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

@danbev build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/88/pipeline

@nodejs-github-bot nodejs-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
Contributor Author

danbev commented Jun 18, 2018

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

@BridgeAR BridgeAR 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
Contributor Author

danbev commented Jun 18, 2018

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)
  ...

bnoordhuis
bnoordhuis approved these changes Jun 18, 2018
View reviewed changes
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.

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

@danbev
Copy link
Contributor Author

danbev commented Jun 19, 2018

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
Contributor Author

danbev commented Jun 19, 2018

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
Contributor Author

danbev commented Jun 20, 2018

Landed in a71d5fc.

@danbev danbev closed this Jun 20, 2018
@danbev danbev 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
@danbev
src: remove .h if -inl.h is already included
a71d5fc 
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 <matheus@sthima.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Ujjwal Sharma <usharma1998@gmail.com>
@targos targos 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 •
edited
Loading

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

targos pushed a commit that referenced this pull request Jul 14, 2018
@danbev @targos
src: remove .h if -inl.h is already included
466601f 
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 <matheus@sthima.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Ujjwal Sharma <usharma1998@gmail.com>
@targos targos mentioned this pull request Jul 17, 2018
Merged
This was referenced Jul 18, 2018
Closed
Closed
This was referenced Jul 18, 2018
Closed
Closed
Closed
@snyk-bot snyk-bot mentioned this pull request Apr 1, 2020
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ryzokuken ryzokuken ryzokuken approved these changes

@BridgeAR BridgeAR BridgeAR approved these changes

@devsnek devsnek devsnek approved these changes

@mmarchini mmarchini mmarchini approved these changes

@cjihrig cjihrig cjihrig approved these changes

@TimothyGu TimothyGu TimothyGu approved these changes

@lpinca lpinca lpinca approved these changes

@bnoordhuis bnoordhuis bnoordhuis approved these changes

Assignees
No one assigned
Labels
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.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.