src: use CreateEnvironment instead of inlining its code where possible#45886

addaleax · 2022-12-16T16:30:38Z

We had a number of places in which we created an Environment instance by performing each step in CreateEnvironment manually. Instead, just call the function itself.

We had a number of places in which we created an `Environment` instance by performing each step in `CreateEnvironment` manually. Instead, just call the function itself.

nodejs-github-bot · 2022-12-16T16:30:41Z

Review requested:

@nodejs/startup

nodejs-github-bot · 2022-12-16T16:33:23Z

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

src/api/environment.cc

src/node_snapshotable.cc

…possible

… where possible

addaleax · 2022-12-16T18:26:35Z

src/api/environment.cc

-inspector_parent_handle.get())->impl));
+ env->InitializeInspector(std::move(
+static_cast<InspectorParentHandleImpl*>(inspector_parent_handle.get())
+ ->impl));


This is now just a clang-format leftover after addressing review comments, I’d leave it if it’s all the same to everyone.

nodejs-github-bot · 2022-12-19T17:11:52Z

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

nodejs-github-bot · 2022-12-19T21:45:53Z

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

nodejs-github-bot · 2022-12-20T10:44:42Z

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

nodejs-github-bot · 2022-12-20T17:42:29Z

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

nodejs-github-bot · 2022-12-21T10:41:03Z

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

nodejs-github-bot · 2022-12-21T16:21:50Z

Landed in 01323d5

We had a number of places in which we created an `Environment` instance by performing each step in `CreateEnvironment` manually. Instead, just call the function itself. PR-URL: #45886 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Chengzhong Wu <[email protected]> Reviewed-By: Joyee Cheung <[email protected]>

juanarbol · 2023-01-22T21:26:50Z

This is not landing cleanly in v18.x, it is generating the. next compile errors:

../src/node_snapshotable.cc:1174:16: error: use of undeclared identifier 'ExitCode' return ExitCode::kBootstrapFailure; ^ ../src/node_snapshotable.cc:1277:73: error: function definition is not allowed here const std::vector<std::string> exec_args){^ ../src/node_snapshotable.cc:1287:21: error: qualified reference to 'SnapshotableObject' is a constructor name rather than a type in this context SnapshotableObject::SnapshotableObject(Environment* env, ^ ../src/node_snapshotable.cc:1287:40: error: 'Environment' does not refer to a value SnapshotableObject::SnapshotableObject(Environment* env, ^ ../src/node_v8.h:13:7: note: declared here class Environment; ^ ../src/node_snapshotable.cc:1288:54: error: expected '(' for function-style cast or type construction Local<Object> wrap, ~~~~~~~~~~~~~ ^ ../src/node_snapshotable.cc:1289:40: error: 'EmbedderObjectType' does not refer to a value EmbedderObjectType type) ^ ../src/node_snapshotable.h:32:12: note: declared here enum class EmbedderObjectType : uint8_t{^ ../src/node_snapshotable.cc:1518:54: error: no member named 'mksnapshot' in namespace 'node' NODE_MODULE_CONTEXT_AWARE_INTERNAL(mksnapshot, node::mksnapshot::Initialize) ~~~~~~^ ../src/node_binding.h:48:42: note: expanded from macro 'NODE_MODULE_CONTEXT_AWARE_INTERNAL' NODE_MODULE_CONTEXT_AWARE_CPP(modname, regfunc, nullptr, NM_F_INTERNAL) ^~~~~~~ ../src/node_binding.h:34:43: note: expanded from macro 'NODE_MODULE_CONTEXT_AWARE_CPP' (node::addon_context_register_func)(regfunc), \ ^~~~~~~ ../src/node_snapshotable.cc:1520:38: error: no member named 'mksnapshot' in namespace 'node' node::mksnapshot::RegisterExternalReferences) ~~~~~~^ ../src/node_external_reference.h:148:5: note: expanded from macro 'NODE_MODULE_EXTERNAL_REFERENCE' func(registry); \ ^~~~ ../src/node_snapshotable.cc:1520:77: error: expected '}' node::mksnapshot::RegisterExternalReferences) ^ ../src/node_snapshotable.cc:27:16: note: to match this '{' namespace node{^ 9 errors generated.

addaleax · 2023-01-24T12:41:38Z

@juanarbol opened #46330 👍

We had a number of places in which we created an `Environment` instance by performing each step in `CreateEnvironment` manually. Instead, just call the function itself. PR-URL: nodejs#45886 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Chengzhong Wu <[email protected]> Reviewed-By: Joyee Cheung <[email protected]>

We had a number of places in which we created an `Environment` instance by performing each step in `CreateEnvironment` manually. Instead, just call the function itself. PR-URL: #45886 Backport-PR-URL: #46330 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Chengzhong Wu <[email protected]> Reviewed-By: Joyee Cheung <[email protected]>

src: use CreateEnvironment instead of inlining its code where possible
bbd1751
We had a number of places in which we created an `Environment` instance by performing each step in `CreateEnvironment` manually. Instead, just call the function itself.

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. labels Dec 16, 2022

addaleax mentioned this pull request Dec 16, 2022
src: add snapshot support for embedder API #45888
Closed

addaleax added the request-ci Add this label to start a Jenkins CI on a PR. label Dec 16, 2022

github-actionsbot removed the request-ci Add this label to start a Jenkins CI on a PR. label Dec 16, 2022

cjihrig approved these changes Dec 16, 2022
View reviewed changes

joyeecheung reviewed Dec 16, 2022
View reviewed changes

src/api/environment.cc Outdated Show resolvedHide resolved

joyeecheung reviewed Dec 16, 2022
View reviewed changes

src/node_snapshotable.cc Outdated Show resolvedHide resolved

addaleax added 2 commits December 16, 2022 19:24

fixup! src: use CreateEnvironment instead of inlining its code where …
6368ff0
…possible

fixup! fixup! src: use CreateEnvironment instead of inlining its code…
b634c38
… where possible

addaleax commented Dec 16, 2022
View reviewed changes

This was referenced Dec 17, 2022
CI Reliability 2022-12-17 nodejs/reliability#460
Open
CI Reliability 2022-12-18 nodejs/reliability#461
Open
CI Reliability 2022-12-19 nodejs/reliability#462
Open

jasnell approved these changes Dec 19, 2022
View reviewed changes

github-actionsbot mentioned this pull request Dec 20, 2022
CI Reliability 2022-12-20 nodejs/reliability#463
Open
14 tasks

legendecas approved these changes Dec 20, 2022
View reviewed changes

addaleax added the request-ci Add this label to start a Jenkins CI on a PR. label Dec 20, 2022

github-actionsbot removed the request-ci Add this label to start a Jenkins CI on a PR. label Dec 20, 2022

github-actionsbot mentioned this pull request Dec 21, 2022
CI Reliability 2022-12-21 nodejs/reliability#464
Open
12 tasks

joyeecheung approved these changes Dec 21, 2022
View reviewed changes

addaleax added commit-queue Add this label to land a pull request using GitHub Actions. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. labels Dec 21, 2022

nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Dec 21, 2022

nodejs-github-bot merged commit 01323d5 into nodejs:mainDec 21, 2022

addaleax deleted the inline-createnevironment branch December 21, 2022 16:26

RafaelGSS mentioned this pull request Jan 2, 2023
v19.4.0 proposal #46061
Merged

juanarbol added the backport-requested-v18.x label Jan 22, 2023

addaleax mentioned this pull request Jan 24, 2023
[v18.x backport] src: use CreateEnvironment instead of inlining its code where possible #46330
Closed

addaleax added backport-open-v18.x and removed backport-requested-v18.x labels Jan 24, 2023

juanarbol mentioned this pull request Mar 3, 2023
V18.15.0 proposal #46920
Merged

Uh oh!

src: use CreateEnvironment instead of inlining its code where possible#45886

src: use CreateEnvironment instead of inlining its code where possible #45886

Uh oh!

Conversation

addaleax commented Dec 16, 2022

Uh oh!

nodejs-github-bot commented Dec 16, 2022

Uh oh!

nodejs-github-bot commented Dec 16, 2022

Uh oh!

Uh oh!

Uh oh!

addaleaxDec 16, 2022

Choose a reason for hiding this comment

Uh oh!

nodejs-github-bot commented Dec 19, 2022

Uh oh!

nodejs-github-bot commented Dec 19, 2022

Uh oh!

nodejs-github-bot commented Dec 20, 2022

Uh oh!

nodejs-github-bot commented Dec 20, 2022

Uh oh!

nodejs-github-bot commented Dec 21, 2022

Uh oh!

nodejs-github-bot commented Dec 21, 2022

Uh oh!

juanarbol commented Jan 22, 2023

Uh oh!

addaleax commented Jan 24, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants