★ wanayoo — archive 1999 https://github.com/nodejs/node/pull/21381Nouvelle recherche | Portail wanayoo
Skip to content

/node

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

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

Closed
wants to merge 1 commit into from

Conversation

Reviewers

@bnoordhuis bnoordhuis

@lpinca lpinca

@TimothyGu TimothyGu

@cjihrig cjihrig

@mmarchini mmarchini

@devsnek devsnek

@BridgeAR BridgeAR

@ryzokuken ryzokuken

Assignees
No one assigned
Labels
C++ lib / src
Projects
None yet
Milestone
No milestone
@danbev
Copy link
Member

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 
This commit removes the normal header file include if an internal one
is specified as per the CPP_STYLE_GUIDE.
Loading status checks…
415aa2e
@nodejs-github-bot

@nodejs-github-bot nodejs-github-bot added C++ lib / src labels Jun 18, 2018

@danbev

This comment has been minimized.

Sign in to view
Copy link
Member Author

danbev commented Jun 18, 2018

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

mmarchini approved these changes Jun 18, 2018
View changes

@devsnek

devsnek approved these changes Jun 18, 2018
View changes

@lpinca

lpinca approved these changes Jun 18, 2018
View changes

@BridgeAR BridgeAR added the author ready label Jun 18, 2018

@BridgeAR

BridgeAR approved these changes Jun 18, 2018
View changes

@danbev

This comment has been minimized.

Sign in to view
Copy link
Member 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)
  ...
@cjihrig

cjihrig approved these changes Jun 18, 2018
View changes

@bnoordhuis

bnoordhuis approved these changes Jun 18, 2018
View changes

Copy link
Member

bnoordhuis left a comment

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

TimothyGu approved these changes Jun 18, 2018
View changes

@ryzokuken

ryzokuken approved these changes Jun 18, 2018
View changes

@danbev

This comment has been minimized.

Sign in to view
Copy link
Member 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

This comment has been minimized.

Sign in to view
Copy link
Member

ryzokuken commented Jun 19, 2018

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

This comment has been minimized.

Sign in to view
Copy link
Member 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

This comment has been minimized.

Sign in to view
Copy link
Member Author

danbev commented Jun 20, 2018

Landed in a71d5fc.

@danbev danbev closed this Jun 20, 2018

@danbev danbev deleted the danbev:remove-header-when-internal-is-used branch Jun 20, 2018

danbev added a commit that referenced this pull request Jun 20, 2018

@danbev
src: remove .h if -inl.h is already included 
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>
Loading status checks…
a71d5fc

@targos targos added backport-requested-v10.x and removed author ready labels Jun 20, 2018

@targos

This comment has been minimized.

Sign in to view
Copy link
Member

targos commented Jun 20, 2018 •
edited

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

@targos targos removed the backport-requested-v10.x label Jul 14, 2018

targos added a commit that referenced this pull request Jul 14, 2018

@danbev @targos
src: remove .h if -inl.h is already included 
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>
Verified
This commit was signed with a verified signature.
@targos targos Michaël Zasso
GPG key ID: 770F7A9A5AE15600 Learn about signing commits
466601f

@targos targos referenced this pull request Jul 17, 2018

Merged

v10.7.0 proposal #21851

This was referenced Jul 18, 2018

Closed
Closed

This was referenced Jul 18, 2018

Closed
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.