src: move C++ binding/addon related code into node_binding{.h, .cc} #24701

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 <[email protected]> Reviewed-By: Richard Lau <[email protected]>

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]>

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 · 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 bnoordhuis, addaleax, 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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

src: move C++ binding/addon related code into node_binding{.h, .cc} #24701

src: move C++ binding/addon related code into node_binding{.h, .cc} #24701

joyeecheung commented Nov 28, 2018

joyeecheung commented Nov 28, 2018

richardlau Nov 28, 2018

richardlau Nov 28, 2018

bnoordhuis left a comment

bnoordhuis Nov 29, 2018

joyeecheung Nov 29, 2018

joyeecheung commented Nov 29, 2018 •

edited

Loading

Trott commented Nov 29, 2018

joyeecheung commented Nov 30, 2018

joyeecheung commented Nov 30, 2018 •

edited

Loading

Trott commented Dec 1, 2018 •

edited

Loading

Trott commented Dec 1, 2018

Trott commented Dec 1, 2018

BethGriggs commented Feb 12, 2019

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

src: move C++ binding/addon related code into node_binding{.h, .cc} #24701

src: move C++ binding/addon related code into node_binding{.h, .cc} #24701

Conversation

joyeecheung commented Nov 28, 2018

Checklist

joyeecheung commented Nov 28, 2018

richardlau Nov 28, 2018

Choose a reason for hiding this comment

richardlau Nov 28, 2018

Choose a reason for hiding this comment

bnoordhuis left a comment

Choose a reason for hiding this comment

bnoordhuis Nov 29, 2018

Choose a reason for hiding this comment

joyeecheung Nov 29, 2018

Choose a reason for hiding this comment

joyeecheung commented Nov 29, 2018 • edited Loading

Trott commented Nov 29, 2018

joyeecheung commented Nov 30, 2018

joyeecheung commented Nov 30, 2018 • edited Loading

Trott commented Dec 1, 2018 • edited Loading

Trott commented Dec 1, 2018

Trott commented Dec 1, 2018

BethGriggs commented Feb 12, 2019

joyeecheung commented Nov 29, 2018 •

edited

Loading

joyeecheung commented Nov 30, 2018 •

edited

Loading

Trott commented Dec 1, 2018 •

edited

Loading