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.

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: reduce number of exported symbols #12366

Closed
wants to merge 1 commit into from

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.
@addaleax addaleax 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-bot nodejs-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
@addaleax addaleax 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
Member Author

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
Member Author

@refack
Copy link
Contributor

refack commented Apr 13, 2017

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
Member Author

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 <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
@addaleax
Copy link
Member Author

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)

@addaleax addaleax closed this Apr 14, 2017
@addaleax addaleax 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.
@addaleax addaleax 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 <info@bnoordhuis.nl>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
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#12366
Fixes: nodejs#12463
@mhdawson mhdawson 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: #12464
Fixes: #12463
Ref: #12366
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Alexey Orlenko <eaglexrlnk@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Brian White <mscdex@mscdex.net>
@jasnell jasnell mentioned this pull request May 11, 2017
Sign up for free to 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 / src Issues 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