FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

[v8.x backport] src: do not include x.h if x-inl.h is included by joyeecheung · Pull Request #16609 · nodejs/node · GitHub

/ node Public

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

Closed
joyeecheung wants to merge 3 commits into
nodejs:v8.x-stagingfrom
joyeecheung:backport-16548-to-v8.x
ConversationCommits (3)ChecksFiles changed
Closed

[v8.x backport] src: do not include x.h if x-inl.h is included #16609
joyeecheung wants to merge 3 commits into
nodejs:v8.x-stagingfrom
joyeecheung:backport-16548-to-v8.x

Conversation

joyeecheung commented Oct 30, 2017 •
edited
Loading

Copy link
Copy Markdown
Member

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 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 commented Oct 30, 2017

Copy link
Copy Markdown
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 force-pushed the backport-16548-to-v8.x branch from 86be7d8 to fd4d8dd Compare October 30, 2017 10:35

joyeecheung commented Oct 30, 2017

Copy link
Copy Markdown
Member Author

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

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

gibfahn commented Oct 30, 2017

Copy link
Copy Markdown
Member

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 commented Oct 30, 2017

Copy link
Copy Markdown
Member

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 closed this Oct 30, 2017
gibfahn reopened this Nov 2, 2017

gibfahn commented Nov 2, 2017

Copy link
Copy Markdown
Member

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

joyeecheung commented Nov 2, 2017

Copy link
Copy Markdown
Member Author

@gibfahn Yes, I'll do a rebase

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>
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>
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 force-pushed the backport-16548-to-v8.x branch from fd4d8dd to d735be9 Compare November 2, 2017 14:59

joyeecheung commented Nov 2, 2017

Copy link
Copy Markdown
Member Author

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

MylesBorins commented Nov 14, 2017

Copy link
Copy Markdown
Contributor

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

gibfahn pushed a commit that referenced this pull request Nov 14, 2017
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
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
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 commented Nov 14, 2017

Copy link
Copy Markdown
Member

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

gibfahn closed this Nov 14, 2017

joyeecheung commented Nov 14, 2017

Copy link
Copy Markdown
Member Author

@MylesBorins v6.x backport is at #16610

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
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

Back | FazBrowse Home | New Git URL