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: move C++ binding/addon related code into node_binding{.h, .cc} #24701

Closed
wants to merge 2 commits into from

Conversation

joyeecheung
Copy link
Member

This patch:

  • Moves the C++ binding/addon related code out of
    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.
  • Introduce a node::binding namespace so that code exposed to
    other files can be easily recognized.
Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

This patch:

- Moves the C++ binding/addon related code out of
  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.
- Introduce a node::binding namespace so that code exposed to
  other files can be easily recognized.
@joyeecheung
Copy link
Member Author


// 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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// Then in node::RegisterBuiltinModules(), it calls modules' registration
// Then in binding::RegisterBuiltinModules(), it calls modules' registration

?

// 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.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// node::RegisterBuiltinModules(). This is only forward declaration.
// binding::RegisterBuiltinModules(). This is only forward declaration.

?

Copy link
Member

@bnoordhuis bnoordhuis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

^ These can and should be static, right? They're only used in this compilation unit.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bnoordhuis Yes, only node_is_initialized is actually used elsewhere, I'll update

@joyeecheung
Copy link
Member Author

joyeecheung commented Nov 29, 2018

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 node::binding namespace, but then I realize it's probably going to worth it if we introduce a namespace dedicated to all these global variables scattered around (e.g. namespace per_process), it would be easier to search for them and recognize them in the code.

@Trott
Copy link
Member

Trott commented Nov 29, 2018

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 joyeecheung added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. addons Issues and PRs related to native addons. c++ Issues and PRs that require attention from people who are familiar with C++. labels Nov 30, 2018
@joyeecheung
Copy link
Member Author

The pis were still unhappy. Full rebuild: https://ci.nodejs.org/job/node-test-pull-request/19080/

@joyeecheung
Copy link
Member Author

joyeecheung commented Nov 30, 2018

@Trott
Copy link
Member

Trott commented Dec 1, 2018

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/

@Trott
Copy link
Member

Trott commented Dec 1, 2018

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/

@Trott
Copy link
Member

Trott commented Dec 1, 2018

Landed in 3d66826

@Trott Trott closed this Dec 1, 2018
Trott pushed a commit to Trott/io.js that referenced this pull request Dec 1, 2018
This patch:

- Moves the C++ binding/addon related code out of
  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.
- Introduce a node::binding namespace so that code exposed to
  other files can be easily recognized.

PR-URL: nodejs#24701
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
BridgeAR pushed a commit that referenced this pull request Dec 5, 2018
This patch:

- Moves the C++ binding/addon related code out of
  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.
- Introduce a node::binding namespace so that code exposed to
  other files can be easily recognized.

PR-URL: #24701
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
@BridgeAR BridgeAR mentioned this pull request Dec 5, 2018
4 tasks
refack pushed a commit to refack/node that referenced this pull request Jan 14, 2019
This patch:

- Moves the C++ binding/addon related code out of
  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.
- Introduce a node::binding namespace so that code exposed to
  other files can be easily recognized.

PR-URL: nodejs#24701
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
@BethGriggs
Copy link
Member

This change does not land cleanly on v10.x-staging. I've added the backport-requested-v10.x label, but feel free to swap to dont-land-on- if this change shouldn't land on v10.x.

codebytere added a commit to electron/electron that referenced this pull request May 27, 2020
codebytere added a commit to electron/electron that referenced this pull request Jun 1, 2020
jkleinsc pushed a commit to electron/electron that referenced this pull request Jun 8, 2020
jkleinsc pushed a commit to electron/electron that referenced this pull request Jun 9, 2020
codebytere added a commit to electron/electron that referenced this pull request Jun 15, 2020
codebytere added a commit to electron/electron that referenced this pull request Jun 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
addons Issues and PRs related to native addons. author ready PRs that have at least one approval, no pending requests for changes, and a CI started. c++ Issues and PRs that require attention from people who are familiar with C++.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants