-
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: remove util-inl.h from header files #27631
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rubber stamp LGTM
@joyeecheung Can you take a look at this, which I just added? I discovered that according to test_inspector_socket_server.cc, an include of node.h used to make the ContainerOf function from util-inl.h available, so simply removing it from the .h files is likely API breaking. Since its not implicitly/transitively included by node.h anymore, I think it needs to be explicitly included when NODE_WANT_INTERNALS is not defined. Perhaps I should do the same for env-inl.h? |
764df0c
to
7b8cddf
Compare
7b8cddf
to
dabf771
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, but needs a rebase.
I'll rebase. It also can't land until I get it working portably, it seems some compiler chains we use handle inlines a bit differently. Once its building clean on all platforms I'll land (but obviously not before). |
dabf771
to
e0ff755
Compare
e0ff755
to
fe6a038
Compare
Its intended that *-inl.h header files are only included into the src files that call the inline methods. Explicitly include it into the files that need it.
fe6a038
to
9f04523
Compare
Landed in 1df3080...b6bfc19 |
PR-URL: #27631 Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
PR-URL: #27631 Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Its intended that *-inl.h header files are only included into the src files that call the inline methods. Explicitly include it into the files that need it. PR-URL: #27631 Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
PR-URL: #27631 Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
PR-URL: #27631 Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Its intended that *-inl.h header files are only included into the src files that call the inline methods. Explicitly include it into the files that need it. PR-URL: #27631 Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
`node.h` may only include public APIs, which `util-inl.h` is not. There does not seem to be any reason for including it, so remove it, because otherwise native addon compilation is broken due to us not shipping the `util-inl.h` header. Refs: nodejs#27631 Fixes: nodejs#27803
`node.h` may only include public APIs, which `util-inl.h` is not. There does not seem to be any reason for including it, so remove it, because otherwise native addon compilation is broken due to us not shipping the `util-inl.h` header. Refs: #27631 Fixes: #27803 PR-URL: #27804 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Sam Roberts <vieuxtech@gmail.com> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com> Reviewed-By: Richard Lau <riclau@uk.ibm.com>
`node.h` may only include public APIs, which `util-inl.h` is not. There does not seem to be any reason for including it, so remove it, because otherwise native addon compilation is broken due to us not shipping the `util-inl.h` header. Refs: #27631 Fixes: #27803 PR-URL: #27804 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Sam Roberts <vieuxtech@gmail.com> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com> Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Also, declared an unused priv argument for Initialize, so it doesn't warn on signature mismatch.
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passesRelated to #27531