★ wanayoo — archive 1999 https://github.com/nodejs/node/pull/24701Nouvelle recherche | Portail wanayoo
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

Projects
None yet
5 participants
@joyeecheung
Copy link
Member

commented Nov 28, 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.
Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
src: move C++ binding/addon related code into node_binding{.h, .cc}
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

This comment has been minimized.

Copy link
Member Author

commented 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.

Copy link
@richardlau

richardlau Nov 28, 2018

Member
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.

This comment has been minimized.

Copy link
@richardlau

richardlau Nov 28, 2018

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

?

@bnoordhuis
Copy link
Member

left a comment

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.

Copy link
@bnoordhuis

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.

Copy link
@joyeecheung

joyeecheung Nov 29, 2018

Author Member

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

@joyeecheung

This comment has been minimized.

Copy link
Member Author

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

This comment has been minimized.

Copy link
Member

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

This comment has been minimized.

Copy link
Member Author

commented Nov 30, 2018

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

@joyeecheung

This comment has been minimized.

@Trott

This comment has been minimized.

Copy link
Member

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

This comment has been minimized.

Copy link
Member

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

This comment has been minimized.

Copy link
Member

commented Dec 1, 2018

Landed in 3d66826

@Trott Trott closed this Dec 1, 2018

Trott added a commit to Trott/io.js that referenced this pull request Dec 1, 2018

src: move C++ binding/addon related code into node_binding{.h, .cc}
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 <info@bnoordhuis.nl>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>

BridgeAR added a commit that referenced this pull request Dec 5, 2018

src: move C++ binding/addon related code into node_binding{.h, .cc}
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 <info@bnoordhuis.nl>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>

@BridgeAR BridgeAR referenced this pull request Dec 5, 2018

Merged

v11.4.0 proposal #24854

4 of 4 tasks complete

refack added a commit to refack/node that referenced this pull request Jan 14, 2019

src: move C++ binding/addon related code into node_binding{.h, .cc}
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 <info@bnoordhuis.nl>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
@BethGriggs

This comment has been minimized.

Copy link
Member

commented Feb 12, 2019

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.