Skip to content

Conversation

@addaleax
Copy link
Member

Use static definitions and anonymous namespaces to reduce the
number of symbols that are exported from the node binary.

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

src

CI: https://ci.nodejs.org/job/node-test-commit/9057/
CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/712/

Use `static` definitions and anonymous namespaces to reduce the number of symbols that are exported from the `node` binary.
@addaleaxaddaleax 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 Apr 12, 2017
@nodejs-github-botnodejs-github-bot added buffer Issues and PRs related to the buffer subsystem. build Issues and PRs related to build files or the CI. c++ Issues and PRs that require attention from people who are familiar with C++. cares Issues and PRs related to the c-ares dependency or the cares_wrap binding. child_process Issues and PRs related to the child_process subsystem. fs Issues and PRs related to the fs subsystem / file system. http_parser Issues and PRs related to the HTTP Parser dependency or the http_parser binding. i18n-api Issues and PRs related to the i18n implementation. libuv Issues and PRs related to the libuv dependency or the uv binding. node-api Issues and PRs related to the Node-API. process Issues and PRs related to the process subsystem. timers Issues and PRs related to the timers subsystem / setImmediate, setInterval, setTimeout. vm Issues and PRs related to the vm subsystem. zlib Issues and PRs related to the zlib subsystem. labels Apr 12, 2017
@addaleaxaddaleax removed buffer Issues and PRs related to the buffer subsystem. build Issues and PRs related to build files or the CI. cares Issues and PRs related to the c-ares dependency or the cares_wrap binding. child_process Issues and PRs related to the child_process subsystem. fs Issues and PRs related to the fs subsystem / file system. http_parser Issues and PRs related to the HTTP Parser dependency or the http_parser binding. i18n-api Issues and PRs related to the i18n implementation. libuv Issues and PRs related to the libuv dependency or the uv binding. node-api Issues and PRs related to the Node-API. process Issues and PRs related to the process subsystem. timers Issues and PRs related to the timers subsystem / setImmediate, setInterval, setTimeout. vm Issues and PRs related to the vm subsystem. zlib Issues and PRs related to the zlib subsystem. labels Apr 12, 2017
@addaleax
Copy link
MemberAuthor

Any chance this will break a "rouge" embedder?

Only if they were relying on these symbols on purpose – so far we’ve always been taking the liberty of changing code that’s guarded by NODE_WANT_INTERNALS or completely local to our source files and didn’t hear anybody complain.

@addaleax
Copy link
MemberAuthor

@refack
Copy link
Contributor

Only if they were relying on these symbols on purpose

That's why I dubbed them "rogue"... I've been know to use undocumented entry points on occasion 😞... You know then might disappear but then you're sad...
Although it's obviously semver-minor if at all, is there a way to put a big warning in the release notes, or land this only in node8?

@addaleax
Copy link
MemberAuthor

addaleax commented Apr 13, 2017

I've been know to use undocumented entry points on occasion

Everything I have seen so far indicates that our embedders and addon developers avoid doing that. (Also, I strongly doubt anything in here would be useful for outside users.)

Although it's obviously semver-minor if at all, is there a way to put a big warning in the release notes, or land this only in node8?

This doesn’t touch our public API, so it’s semver-nothing, and I really don’t think this is worth an explicit mention in the release notes. If you feel really strongly about it, we can only land it in Node 8, I don’t care too much.

(edited a bit later)

@refack
Copy link
Contributor

refack commented Apr 13, 2017

This doesn’t touch our public API, so it’s semver-nothing, and I really don’t think this is worth an explicit mention in the release notes. If you feel really strongly about it, we can only land it in Node 8, I don’t care too much.

I agree it's semver-nothing, and I don't feel too strongly one way or another.
But I do think it's worth a special mention, writing comments in the release notes is easier than fielding issues from frustrated (even if misguided) users.

Just an example in the description of #8749 is says "Any non-null value of NODE_PRESERVE_SYMLINKS will enable symlinks." even tough the code acts differently and is documented correctly, that caused me a few hours of frustration, with no-one to blame but myself...

@benjamingr
Copy link
Member

LGTM if citgm is green. I would prefer a semver major because if embedders but honestly I've never heard of anyone doing that before and I won't block this.

addaleax added a commit to addaleax/node that referenced this pull request Apr 14, 2017
Use `static` definitions and anonymous namespaces to reduce the number of symbols that are exported from the `node` binary. PR-URL: nodejs#12366 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: James M Snell <[email protected]>
@addaleax
Copy link
MemberAuthor

addaleax commented Apr 14, 2017

Landed in 9d52222

I would prefer a semver major because if embedders but honestly I've never heard of anyone doing that before and I won't block this.

@benjamingr Like I’ve said, we’ve been quite liberal with internals so far, so it would be very surprising if this broke some actual code. I’ve added the dont-land labels because it shouldn’t really matter, but I would like to avoid getting in a habit of considering these actual breaking changes.

(edit: if this makes it into a release < 8.x it should probably come together with #12432)

@addaleaxaddaleax deleted the src-static-anon branch April 14, 2017 21:09
addaleax added a commit to addaleax/node that referenced this pull request Apr 15, 2017
Overlooked in nodejs#12366. Removing this removes a compiler warning.
@addaleaxaddaleax mentioned this pull request Apr 15, 2017
2 tasks
addaleax added a commit that referenced this pull request Apr 16, 2017
Overlooked in #12366. Removing this removes a compiler warning. PR-URL: #12432 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]>
addaleax added a commit to addaleax/node that referenced this pull request Apr 17, 2017
In 9d52222 the indirect "http_parser.h" include was removed, which made `NODE_STRINGIFY()` fail silently for the http parser version in `process.versions`. Ref: nodejs#12366Fixes: nodejs#12463
@mhdawsonmhdawson mentioned this pull request Apr 18, 2017
2 tasks
jasnell pushed a commit that referenced this pull request Apr 19, 2017
In 9d52222 the indirect "http_parser.h" include was removed, which made `NODE_STRINGIFY()` fail silently for the http parser version in `process.versions`. PR-URL: #12464Fixes: #12463 Ref: #12366 Reviewed-By: Evan Lucas <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Alexey Orlenko <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Brian White <[email protected]>
@jasnelljasnell mentioned this pull request May 11, 2017
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

c++Issues and PRs that require attention from people who are familiar with C++.lib / srcIssues and PRs related to general changes in the lib or src directory.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants

@addaleax@refack@benjamingr@bnoordhuis@jasnell@nodejs-github-bot