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

src: do not include internals from node_buffer.h by addaleax · Pull Request #15554 · nodejs/node · GitHub

/ node Public

src: do not include internals from node_buffer.h - #15554

Closed
addaleax wants to merge 1 commit into
nodejs:masterfrom
addaleax:fix-buffer-include
ConversationCommits (1)ChecksFiles changed
Closed

src: do not include internals from node_buffer.h#15554
addaleax wants to merge 1 commit into
nodejs:masterfrom
addaleax:fix-buffer-include

Conversation

addaleax commented Sep 22, 2017

Copy link
Copy Markdown
Member

node_buffer.h is a public header, so it should not be using the node_internals.h internal header.

Ref: 290315a
Fixes: #15552

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

addons

src: do not include internals from node_buffer.h
6dcf62c 
`node_buffer.h` is a public header, so it should not be using
the `node_internals.h` internal header.

Ref: 290315a
Fixes: nodejs#15552
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 Sep 22, 2017

addaleax commented Sep 22, 2017

Copy link
Copy Markdown
Member Author

CI: https://ci.nodejs.org/job/node-test-commit/12515/

@nodejs/collaborators I would like to fast-track this to unbreak addons.

This really should have been caught by our addon tests, and I’m working on fixing that, but it’s going to be trickier than the fix and probably shouldn’t be rushed.

AndreasMadsen commented Sep 22, 2017

Copy link
Copy Markdown
Member

the v8.x branch works. I don't think it is strictly necessary to fast track this but I'm not against it.

addaleax commented Sep 22, 2017

Copy link
Copy Markdown
Member Author

@AndreasMadsen Maybe I’m misunderstanding that last comment, but as @richardlau pointed out in #15509, the bug would be “about” to break 8.6.0.

I don’t think this needs to land asap, but it would be nice to not have to wait the full 72 hours (if only because of the broken nightlies).

AndreasMadsen commented Sep 22, 2017

Copy link
Copy Markdown
Member

@addaleax I think you understood me fine.

mcollina approved these changes Sep 22, 2017
View reviewed changes

mcollina left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM expedited.

richardlau commented Sep 22, 2017

Copy link
Copy Markdown
Member

This really should have been caught by our addon tests, and I’m working on fixing that, but it’s going to be trickier than the fix and probably shouldn’t be rushed.

Perhaps we can revive #12231.

addaleax commented Sep 22, 2017

Copy link
Copy Markdown
Member Author

@richardlau thanks, I didn’t remember that – it’s basically exactly the kind of test change we need …

addaleax commented Sep 22, 2017

Copy link
Copy Markdown
Member Author

Landed in 84063a7

addaleax closed this Sep 22, 2017
addaleax deleted the fix-buffer-include branch September 22, 2017 14:30
addaleax added a commit that referenced this pull request Sep 22, 2017
src: do not include internals from node_buffer.h
84063a7 
`node_buffer.h` is a public header, so it should not be using
the `node_internals.h` internal header.

Ref: 290315a
Fixes: #15552
PR-URL: #15554
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Andreas Madsen <amwebdk@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
addaleax added a commit to addaleax/ayo that referenced this pull request Sep 23, 2017
src: do not include internals from node_buffer.h
df18f2a 
`node_buffer.h` is a public header, so it should not be using
the `node_internals.h` internal header.

Ref: 290315a
Fixes: nodejs/node#15552
PR-URL: nodejs/node#15554
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Andreas Madsen <amwebdk@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
jasnell pushed a commit that referenced this pull request Sep 25, 2017
src: do not include internals from node_buffer.h
24271a7 
`node_buffer.h` is a public header, so it should not be using
the `node_internals.h` internal header.

Ref: 290315a
Fixes: #15552
PR-URL: #15554
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Andreas Madsen <amwebdk@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>

MylesBorins commented Oct 17, 2017

Copy link
Copy Markdown
Contributor

as #14697 did not land on v6.x I'm setting this to dont-land-on-v6.x lmk if this is a mistake

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

mcollina mcollina approved these changes
cjihrig cjihrig approved these changes
tniessen tniessen approved these changes
richardlau richardlau approved these changes
+2 more reviewers
AndreasMadsen AndreasMadsen approved these changes
gibfahn gibfahn approved these changes
Reviewers whose approvals may not affect merge requirements

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.

fatal error: 'node_internals.h' file not found

9 participants

Back | FazBrowse Home | New Git URL