src: use CreateEnvironment instead of inlining its code where possible by addaleax · Pull Request #45886 · nodejs/node

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/

…possible

… where possible

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

-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 <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Chengzhong Wu <legendecas@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>

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 <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Chengzhong Wu <legendecas@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>

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 <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Chengzhong Wu <legendecas@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>

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

Comment threadsrc/api/environment.cc Outdated

joyeecheung reviewed Dec 16, 2022
View reviewed changes

Comment threadsrc/node_snapshotable.cc Outdated

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

Navigation Menu

Uh oh!

Uh oh!

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

src: use CreateEnvironment instead of inlining its code where possible#45886
nodejs-github-bot merged 3 commits into
nodejs:mainfrom
addaleax:inline-createnevironment

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

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

Uh oh!

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