src: include specific V8 headers in node_v8 #40427
Conversation
nodejs-github-bot
added
c++
|
/cc @addaleax, @jasnell, @joyeecheung (I always miss that we don't have a What do you think about this in general? Should we try to migrate everything in |
|
Ouch, that's going to make things more complicated. The only concern I'd have with this goes to ABI stability. In most headers I think we're fine but, as you mention, node.h is definitely problematic, and yes, it will likely need to be semver major. |
|
If we think that the complexity is not worth the potential improvements, I can also close this and the referenced issue |
|
I'd say doing internal cleanup is okay, we could probably use a guard behind Tangentially, is something similar being done to NAN headers? |
targos
changed the title
|
@joyeecheung I tried to apply your suggestion. To do so, I started from another file that seemed more self-contained (I was wrong, it's not):
Not sure I understand the question. I am not involved in the NAN project. |
src/node.h
Outdated
| #ifndef NODE_WANT_V8_SUBHEADERS | ||
| #include "v8.h" | ||
| #else | ||
| #include "v8-array-buffer.h" |
There was a problem hiding this comment.
With NODE_WANT_V8_SUBHEADERS, the list in the else clause should be empty - if node.h still includes the subheaders when NODE_WANT_V8_SUBHEADERS is defined, then node_v8.cc can still compile without including them specifically.
There was a problem hiding this comment.
Shouldn't we still include the headers that node.h needs? It seems wrong to have to do it from outside
There was a problem hiding this comment.
Ah, right, we need this for the stuff in node.h too, I think in that case it should be fine to omit what's already in node.h in other files, too, since we've always been doing that
There was a problem hiding this comment.
I just realized that we could also include v8-forward.h instead of the full headers. I'll investigate that
https://github.com/nodejs/node/blob/master/deps/v8/include/v8-forward.h
There was a problem hiding this comment.
Updated. Does it look better now?
Just a general question since removing v8.h from node.h is mentioned, I figured if we do want to do that, starting from NAN might be a first step? |
Also do it in all its dependencies. This introduces the NODE_WANT_V8_SUBHEADERS macro to opt in to only include specific V8 headers from our public ones (e.g. node.h).
|
I'll try to do it with https://include-what-you-use.org/ when I have time. |
Refs: #39876