Perfect your code
With built-in code review tools, GitHub makes it easy to raise the quality bar before you ship. Join the 36 million developers who've merged over 200 million pull requests.
Sign up for free See pricing for teams and enterprisessrc: move C++ binding/addon related code into node_binding{.h, .cc} #24701
Conversation
joyeecheung
requested review from
bnoordhuis,
addaleax,
gabrielschulhof and
yhwang
Nov 28, 2018
This comment has been minimized.
This comment has been minimized.
richardlau
reviewed
Nov 28, 2018
|
|
||
| // A list of built-in modules. In order to do module registration | ||
| // in node::Init(), need to add built-in modules in the following list. | ||
| // Then in node::RegisterBuiltinModules(), it calls modules' registration |
This comment has been minimized.
This comment has been minimized.
richardlau
Nov 28, 2018
Member
| // Then in node::RegisterBuiltinModules(), it calls modules' registration | |
| // Then in binding::RegisterBuiltinModules(), it calls modules' registration |
?
richardlau
reviewed
Nov 28, 2018
| // This is used to load built-in modules. Instead of using | ||
| // __attribute__((constructor)), we call the _register_<modname> | ||
| // function for each built-in modules explicitly in | ||
| // node::RegisterBuiltinModules(). This is only forward declaration. |
This comment has been minimized.
This comment has been minimized.
richardlau
Nov 28, 2018
Member
| // node::RegisterBuiltinModules(). This is only forward declaration. | |
| // binding::RegisterBuiltinModules(). This is only forward declaration. |
?
bnoordhuis
approved these changes
Nov 29, 2018
|
Rubber-stamp LGTM % my and Richard's comments. |
| node_module* modlist_linked; | ||
| node_module* modlist_addon; | ||
| uv_once_t init_modpending_once = UV_ONCE_INIT; | ||
| uv_key_t thread_local_modpending; |
This comment has been minimized.
This comment has been minimized.
bnoordhuis
Nov 29, 2018
Member
^ These can and should be static, right? They're only used in this compilation unit.
This comment has been minimized.
This comment has been minimized.
joyeecheung
Nov 29, 2018
Author
Member
@bnoordhuis Yes, only node_is_initialized is actually used elsewhere, I'll update
This comment has been minimized.
This comment has been minimized.
|
Thanks for the reviews, updated. CI: https://ci.nodejs.org/job/node-test-pull-request/19039/ On a side note: thought about moving the static global variables into the |
richardlau
approved these changes
Nov 29, 2018
This comment has been minimized.
This comment has been minimized.
|
Resume Build CI: https://ci.nodejs.org/job/node-test-pull-request/19052/ (Might benefit from a full CI rebuild to pull in a status file change if the Raspberry Pi rebuild keeps failing...) |
joyeecheung
added
author ready
addons
C++
labels
Nov 30, 2018
This comment has been minimized.
This comment has been minimized.
|
The pis were still unhappy. Full rebuild: https://ci.nodejs.org/job/node-test-pull-request/19080/ |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
I believe the issue with Alpine is resolved now, so let's try CI again: Resume Build CI: https://ci.nodejs.org/job/node-test-pull-request/19102/ |
This comment has been minimized.
This comment has been minimized.
|
Well, the one Alpine issue seems resolved but the other Alpine host seems to have disconnected or something mid-compilation...once more... Resume Build CI: https://ci.nodejs.org/job/node-test-pull-request/19103/ |
This comment has been minimized.
This comment has been minimized.
|
Landed in 3d66826 |
Trott
closed this
Dec 1, 2018
Trott
added a commit
to Trott/io.js
that referenced
this pull request
Dec 1, 2018
BridgeAR
added a commit
that referenced
this pull request
Dec 5, 2018
This was referenced Dec 7, 2018
refack
added a commit
to refack/node
that referenced
this pull request
Jan 14, 2019
This comment has been minimized.
This comment has been minimized.
|
This change does not land cleanly on |
joyeecheung commentedNov 28, 2018
This patch:
node_internals.h/node.cc and into dedicated files
node_binding.h/node_binding.cc, and only puts the code resued
by other files into the header.
other files can be easily recognized.
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passes