src: move C++ binding/addon related code into node_binding{.h, .cc} by joyeecheung · Pull Request #24701 · nodejs/node

joyeecheung · 2018-11-28T17:47:19Z

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 · 2018-11-28T17:48:53Z

CI: https://ci.nodejs.org/job/node-test-pull-request/19007/

richardlau · 2018-11-28T17:52:30Z

src/node_binding.cc

+
+// 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


Suggested change

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

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

?

richardlau · 2018-11-28T17:53:13Z

src/node_binding.cc

+// 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.


Suggested change

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

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

?

bnoordhuis

Rubber-stamp LGTM % my and Richard's comments.

bnoordhuis · 2018-11-29T10:25:22Z

src/node_binding.cc

+node_module* modlist_linked;
+node_module* modlist_addon;
+uv_once_t init_modpending_once = UV_ONCE_INIT;
+uv_key_t thread_local_modpending;


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

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

… .cc}

joyeecheung · 2018-11-29T11:05:49Z

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 · 2018-11-29T14:36:59Z

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 · 2018-11-30T09:25:58Z

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

joyeecheung · 2018-11-30T20:28:37Z

Resume: https://ci.nodejs.org/job/node-test-pull-request/19093/
Another one: https://ci.nodejs.org/job/node-test-pull-request/19101/

Trott · 2018-12-01T02:05:59Z

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 · 2018-12-01T03:06:51Z

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 · 2018-12-01T04:14:13Z

Landed in 3d66826

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>

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>

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 · 2019-02-12T15:19:21Z

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.

nodejs/node#24701

joyeecheung requested review from addaleax, bnoordhuis, gabrielschulhof and yhwang November 28, 2018 17:47

richardlau reviewed Nov 28, 2018

View reviewed changes

bnoordhuis approved these changes Nov 29, 2018

View reviewed changes

fixup! src: move C++ binding/addon related code into node_binding{.h,…

7d77140

… .cc}

richardlau approved these changes Nov 29, 2018

View reviewed changes

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

Trott closed this Dec 1, 2018

BridgeAR mentioned this pull request Dec 5, 2018

v11.4.0 proposal #24854

Merged

4 tasks

BethGriggs added the backport-requested-v10.x label Feb 12, 2019

codebytere added a commit to electron/electron that referenced this pull request May 27, 2020

Fixup hooks & exceptions patch

91ce881

nodejs/node#24701

codebytere added a commit to electron/electron that referenced this pull request Jun 1, 2020

Fixup hooks & exceptions patch

5d9aca1

nodejs/node#24701

jkleinsc pushed a commit to electron/electron that referenced this pull request Jun 8, 2020

Fixup hooks & exceptions patch

41f1e1f

nodejs/node#24701

jkleinsc pushed a commit to electron/electron that referenced this pull request Jun 9, 2020

Fixup hooks & exceptions patch

60e23b0

nodejs/node#24701

codebytere added a commit to electron/electron that referenced this pull request Jun 15, 2020

Fixup hooks & exceptions patch

f29f43b

nodejs/node#24701

codebytere added a commit to electron/electron that referenced this pull request Jun 17, 2020

Fixup hooks & exceptions patch

8897d25

nodejs/node#24701

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

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

Uh oh!

Conversation

joyeecheung commented Nov 28, 2018

Checklist

Uh oh!

joyeecheung commented Nov 28, 2018

Uh oh!

richardlau Nov 28, 2018

Choose a reason for hiding this comment

Uh oh!

richardlau Nov 28, 2018

Choose a reason for hiding this comment

Uh oh!

bnoordhuis left a comment

Choose a reason for hiding this comment

Uh oh!

bnoordhuis Nov 29, 2018

Choose a reason for hiding this comment

Uh oh!

joyeecheung Nov 29, 2018

Choose a reason for hiding this comment

Uh oh!

joyeecheung commented Nov 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Trott commented Nov 29, 2018

Uh oh!

joyeecheung commented Nov 30, 2018

Uh oh!

joyeecheung commented Nov 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Trott commented Dec 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Trott commented Dec 1, 2018

Uh oh!

Trott commented Dec 1, 2018

Uh oh!

BethGriggs commented Feb 12, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

joyeecheung commented Nov 29, 2018 •

edited

Loading

joyeecheung commented Nov 30, 2018 •

edited

Loading

Trott commented Dec 1, 2018 •

edited

Loading