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

[v8.x backport] src: do not include x.h if x-inl.h is included #16609

Closed
wants to merge 3 commits into from
Closed

[v8.x backport] src: do not include x.h if x-inl.h is included #16609

wants to merge 3 commits into from

Conversation

joyeecheung
Copy link
Member

@joyeecheung joyeecheung commented Oct 30, 2017 •
edited
Loading

Refs: #16548
Refs: #16518
Refs: #16639

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)

src, doc

@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. v8.x labels Oct 30, 2017
@joyeecheung
Copy link
Member Author

This is a pre-backport considering the amount of churn this could bring.

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

@joyeecheung joyeecheung force-pushed the backport-16548-to-v8.x branch from 86be7d8 to fd4d8dd Compare October 30, 2017 10:35
@joyeecheung
Copy link
Member Author

Forgot that this depends on #16518 ...new CI: https://ci.nodejs.org/job/node-test-pull-request/11088/

@gibfahn gibfahn force-pushed the v8.x-staging branch 5 times, most recently from b183192 to fc8acc8 Compare October 30, 2017 21:42
@gibfahn
Copy link
Member

gibfahn commented Oct 30, 2017

Thanks for doing this, but this (somehow) cherry-picked cleanly in 13c03ff and fc8acc8.

Could you take a look at 13c03ff ? It seems to have some differences from b20ad2a, and it'd be good to know whether that's an issue, and if so whether we should revert the one in v8.x-staging, and use the one in this PR.

@gibfahn
Copy link
Member

gibfahn commented Oct 30, 2017

Actually had to back this out due to #16622

I think this can be closed. Assuming the revert lands, then you'll probably want to raise a new PR with a fixed version of #16548, and it'd be clearer if that had its own backport PR.

If that doesn't end up happening then we can reopen.

@gibfahn gibfahn closed this Oct 30, 2017
@gibfahn gibfahn reopened this Nov 2, 2017
@gibfahn
Copy link
Member

gibfahn commented Nov 2, 2017

So this needs #16639 bundled in then it should be good to go right?

@joyeecheung
Copy link
Member Author

@gibfahn Yes, I'll do a rebase

@joyeecheung
src: do not include x.h if x-inl.h is included
f6a52a2 
Fixes: nodejs#16519
PR-URL: nodejs#16548
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@joyeecheung
doc: add *-inl.h include rule to C++ style guide
91a2daa 
PR-URL: nodejs#16548
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@joyeecheung
src: fix etw provider include on Windows
d735be9 
PR-URL: nodejs#16639
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
@joyeecheung joyeecheung force-pushed the backport-16548-to-v8.x branch from fd4d8dd to d735be9 Compare November 2, 2017 14:59
@joyeecheung
Copy link
Member Author

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

@MylesBorins
Copy link
Contributor

@joyeecheung should this be backported to v6.x as well?

gibfahn pushed a commit that referenced this pull request Nov 14, 2017
@joyeecheung @gibfahn
src: do not include x.h if x-inl.h is included
255fffb 
Fixes: #16519
PR-URL: #16548
Backport-PR-URL: #16609
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
gibfahn pushed a commit that referenced this pull request Nov 14, 2017
@joyeecheung @gibfahn
doc: add *-inl.h include rule to C++ style guide
a2fd9a3 
PR-URL: #16548
Backport-PR-URL: #16609
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
gibfahn pushed a commit that referenced this pull request Nov 14, 2017
@joyeecheung @gibfahn
src: fix etw provider include on Windows
cecd1e3 
PR-URL: #16639
Backport-PR-URL: #16609
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
@gibfahn
Copy link
Member

gibfahn commented Nov 14, 2017

Landed on v8.x-staging in cecd1e3 a2fd9a3 255fffb

@gibfahn gibfahn closed this Nov 14, 2017
@joyeecheung
Copy link
Member Author

@MylesBorins v6.x backport is at #16610

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
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.
4 participants
@joyeecheung @gibfahn @MylesBorins @nodejs-github-bot