-
Notifications
You must be signed in to change notification settings - Fork 30k
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: add missing 'inline' keywords #6056
Conversation
Apropos |
Okay, |
LGTM |
The BaseObject constructor and destructor should not have external linkage because BaseObject is a header-only construct. Add the necessary 'inline' keywords. PR-URL: nodejs#6056 Reviewed-By: Colin Ihrig <[email protected]>
We don't link with `--gc-sections` because it's unreliable with some toolchains, so all these flags do is make the compiler generate slightly worse code. Drop them. Refs: nodejs#6056 PR-URL: nodejs#6077 Reviewed-By: Trevor Norris <[email protected]>
@bnoordhuis ... does this need to happen in v4 also? |
It's not critical but yes. |
A bit late to the party here but what about the possibility of enabling the flags on known working platforms? |
I didn't run extensive tests but I don't think it's really worth the trouble. It trims about 16K from the 20 MB binary (16 MB stripped) and it seems to negatively impact gcc's ability to do intra-procedural optimizations (maybe not when LTO is enabled but we don't do that, and we wouldn't need the flags in that case anyway.) |
The BaseObject constructor and destructor should not have external linkage because BaseObject is a header-only construct. Add the necessary 'inline' keywords. PR-URL: #6056 Reviewed-By: Colin Ihrig <[email protected]>
We don't link with `--gc-sections` because it's unreliable with some toolchains, so all these flags do is make the compiler generate slightly worse code. Drop them. Refs: #6056 PR-URL: #6077 Reviewed-By: Trevor Norris <[email protected]>
The BaseObject constructor and destructor should not have external linkage because BaseObject is a header-only construct. Add the necessary 'inline' keywords. PR-URL: #6056 Reviewed-By: Colin Ihrig <[email protected]>
We don't link with `--gc-sections` because it's unreliable with some toolchains, so all these flags do is make the compiler generate slightly worse code. Drop them. Refs: #6056 PR-URL: #6077 Reviewed-By: Trevor Norris <[email protected]>
The BaseObject constructor and destructor should not have external linkage because BaseObject is a header-only construct. Add the necessary 'inline' keywords. PR-URL: #6056 Reviewed-By: Colin Ihrig <[email protected]>
We don't link with `--gc-sections` because it's unreliable with some toolchains, so all these flags do is make the compiler generate slightly worse code. Drop them. Refs: #6056 PR-URL: #6077 Reviewed-By: Trevor Norris <[email protected]>
The BaseObject constructor and destructor should not have external linkage because BaseObject is a header-only construct. Add the necessary 'inline' keywords. PR-URL: #6056 Reviewed-By: Colin Ihrig <[email protected]>
The BaseObject constructor and destructor should not have external linkage because BaseObject is a header-only construct. Add the necessary 'inline' keywords. PR-URL: #6056 Reviewed-By: Colin Ihrig <[email protected]>
The BaseObject constructor and destructor should not have external linkage because BaseObject is a header-only construct. Add the necessary 'inline' keywords. PR-URL: #6056 Reviewed-By: Colin Ihrig <[email protected]>
Pull Request check-list
make -j8 test
(UNIX) orvcbuild test nosign
(Windows) pass withthis change (including linting)?
test (or a benchmark) included?
existing APIs, or introduces new ones)?
Affected core subsystem(s)
build, src
Description of change
CI: https://ci.nodejs.org/job/node-test-pull-request/2160/