Skip to content

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

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Dec 21, 2022
Merged

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

merged 3 commits into from
Dec 21, 2022

Conversation

@addaleax
Copy link
Member

@addaleax addaleax commented Dec 16, 2022

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.
@addaleax
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
Copy link
Collaborator

nodejs-github-bot commented Dec 16, 2022

Review requested:

  • @nodejs/startup
@nodejs-github-bot 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 addaleax mentioned this pull request Dec 16, 2022
Closed
@addaleax addaleax added the request-ci Add this label to start a Jenkins CI on a PR. label Dec 16, 2022
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Dec 16, 2022
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Dec 16, 2022

CI: https://ci.nodejs.org/job/node-test-pull-request/48523/
joyeecheung
joyeecheung reviewed Dec 16, 2022
View reviewed changes
joyeecheung
joyeecheung reviewed Dec 16, 2022
View reviewed changes
addaleax
addaleax commented Dec 16, 2022
View reviewed changes
src/api/environment.cc
inspector_parent_handle.get())->impl));
env->InitializeInspector(std::move(
static_cast<InspectorParentHandleImpl*>(inspector_parent_handle.get())
->impl));
Copy link
Member Author

@addaleax addaleax Dec 16, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is now just a clang-format leftover after addressing review comments, I’d leave it if it’s all the same to everyone.
This was referenced Dec 17, 2022
Open
Open
Open
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Dec 19, 2022

CI: https://ci.nodejs.org/job/node-test-pull-request/48595/
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Dec 19, 2022

CI: https://ci.nodejs.org/job/node-test-pull-request/48613/
@github-actions github-actions bot mentioned this pull request Dec 20, 2022
Open
14 tasks
@addaleax addaleax added the request-ci Add this label to start a Jenkins CI on a PR. label Dec 20, 2022
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Dec 20, 2022
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Dec 20, 2022

CI: https://ci.nodejs.org/job/node-test-pull-request/48629/
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Dec 20, 2022

CI: https://ci.nodejs.org/job/node-test-pull-request/48647/
@github-actions github-actions bot mentioned this pull request Dec 21, 2022
Open
12 tasks
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Dec 21, 2022

CI: https://ci.nodejs.org/job/node-test-pull-request/48663/
@addaleax 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 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 nodejs-github-bot merged commit 01323d5 into nodejs:main Dec 21, 2022
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Dec 21, 2022

Landed in 01323d5
@addaleax addaleax deleted the inline-createnevironment branch December 21, 2022 16:26
This was referenced Dec 22, 2022
Open
Open
Open
Open
Open
Open
Open
targos pushed a commit that referenced this pull request Jan 1, 2023
@addaleax @targos
src: use CreateEnvironment instead of inlining its code where possible
a85264b 
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>
@RafaelGSS RafaelGSS mentioned this pull request Jan 2, 2023
Merged
RafaelGSS pushed a commit that referenced this pull request Jan 4, 2023
@addaleax @RafaelGSS
src: use CreateEnvironment instead of inlining its code where possible
aab0eaa 
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>
RafaelGSS pushed a commit that referenced this pull request Jan 5, 2023
@addaleax @RafaelGSS
src: use CreateEnvironment instead of inlining its code where possible
b995138 
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
Copy link
Member

juanarbol commented Jan 22, 2023

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 addaleax mentioned this pull request Jan 24, 2023
Closed
@addaleax
Copy link
Member Author

addaleax commented Jan 24, 2023

@juanarbol opened #46330 👍
addaleax added a commit to addaleax/node that referenced this pull request Jan 24, 2023
@addaleax
src: use CreateEnvironment instead of inlining its code where possible
335f8dc 
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>
addaleax added a commit to addaleax/node that referenced this pull request Feb 9, 2023
@addaleax
src: use CreateEnvironment instead of inlining its code where possible
466d7e2 
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>
juanarbol pushed a commit to addaleax/node that referenced this pull request Feb 24, 2023
@addaleax @juanarbol
src: use CreateEnvironment instead of inlining its code where possible
481bbb4 
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>
juanarbol pushed a commit that referenced this pull request Mar 1, 2023
@addaleax @juanarbol
src: use CreateEnvironment instead of inlining its code where possible
7f0b553 
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>
@juanarbol juanarbol mentioned this pull request Mar 3, 2023
Merged
juanarbol pushed a commit that referenced this pull request Mar 5, 2023
@addaleax @juanarbol
src: use CreateEnvironment instead of inlining its code where possible
8adaa13 
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

c++ Issues and PRs that require attention from people who are familiar with C++. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run.

7 participants

@addaleax @nodejs-github-bot @juanarbol @jasnell @cjihrig @joyeecheung @legendecas