Skip to content
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

src: simplify exit code accesses #45125

Merged
merged 8 commits into from
Nov 10, 2022
Merged

src: simplify exit code accesses #45125

merged 8 commits into from
Nov 10, 2022

Conversation

daeyeon
Copy link
Member

@daeyeon daeyeon commented Oct 22, 2022

This simplifies getting the exit code which is set through process.exitCode by removing
manually reading the JS property from the native side.

Addresses this TODO:

node/src/api/hooks.cc

Lines 72 to 75 in 5815e3e

// TODO(addaleax): It might be nice to share process.exitCode via
// getter/setter pairs that pass data directly to the native side, so that we
// don't manually have to read and write JS properties here. These getters
// could use e.g. a typed array for performance.

Signed-off-by: Daeyeon Jeong daeyeon.dev@gmail.com

@nodejs-github-bot
Copy link
Collaborator

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 Oct 22, 2022
@daeyeon daeyeon added the process Issues and PRs related to the process subsystem. label Oct 22, 2022
@daeyeon
Copy link
Member Author

daeyeon commented Oct 22, 2022

/cc @addaleax @nodejs/cpp-reviewers

@daeyeon daeyeon force-pushed the main.exitcode-with-accessors-221022.Sat.fefa branch from bd891da to a107886 Compare October 22, 2022 12:04
@daeyeon
src: simplify exit code accesses
eef1c86 
This simplifies getting the exit code which is set through
`process.exitCode` by removing manually reading the JS property
from the native side.

Signed-off-by: Daeyeon Jeong <daeyeon.dev@gmail.com>
@daeyeon daeyeon force-pushed the main.exitcode-with-accessors-221022.Sat.fefa branch from a107886 to eef1c86 Compare October 22, 2022 12:13
RaisinTen
RaisinTen reviewed Oct 22, 2022
View reviewed changes
src/node_process_object.cc Outdated Show resolved Hide resolved
src/env_properties.h Outdated Show resolved Hide resolved
@daeyeon
fixup: use typed array
13ef5a2 
Signed-off-by: Daeyeon Jeong <daeyeon.dev@gmail.com>
@daeyeon
Copy link
Member Author

daeyeon commented Oct 29, 2022

Changed the previous commit to use a typed array. PTAL.

@daeyeon daeyeon added the commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. label Oct 29, 2022
RaisinTen
RaisinTen approved these changes Oct 31, 2022
View reviewed changes
Copy link
Contributor

@RaisinTen RaisinTen left a comment

Choose a reason for hiding this comment

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

LGTM!

@daeyeon daeyeon added the request-ci Add this label to start a Jenkins CI on a PR. label Oct 31, 2022
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 31, 2022
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
joyeecheung
joyeecheung reviewed Oct 31, 2022
View reviewed changes
src/env.h Outdated Show resolved Hide resolved
@daeyeon daeyeon force-pushed the main.exitcode-with-accessors-221022.Sat.fefa branch from 204ca1e to 3a854d1 Compare November 4, 2022 15:31
@daeyeon
Copy link
Member Author

daeyeon commented Nov 4, 2022

Updated by removing the new struct in the previous commit and extending the exiting_ buffer. The exiting_ is renamed to exit_info_. PTAL.

@daeyeon
fixup: use inlined typed array
62819b1 
Signed-off-by: Daeyeon Jeong <daeyeon.dev@gmail.com>
@daeyeon daeyeon force-pushed the main.exitcode-with-accessors-221022.Sat.fefa branch from 3a854d1 to 62819b1 Compare November 4, 2022 15:45
daeyeon
daeyeon commented Nov 4, 2022
View reviewed changes
src/env.h Outdated Show resolved Hide resolved
joyeecheung
joyeecheung reviewed Nov 4, 2022
View reviewed changes
lib/internal/bootstrap/node.js Outdated Show resolved Hide resolved
src/env-inl.h Outdated Show resolved Hide resolved
@daeyeon
fixup: export field identifiers
375ea8d 
Signed-off-by: Daeyeon Jeong <daeyeon.dev@gmail.com>
@daeyeon
fixup: change int32_t to ExitCode
fea1885 
Signed-off-by: Daeyeon Jeong <daeyeon.dev@gmail.com>
@daeyeon
Copy link
Member Author

daeyeon commented Nov 5, 2022

Applied the suggestions. PTAL.

RaisinTen
RaisinTen reviewed Nov 5, 2022
View reviewed changes
src/env.h Outdated Show resolved Hide resolved
src/env.cc Outdated Show resolved Hide resolved
@daeyeon
fixup: add ExitInfoFieldCount and revert unrelated change
a3d9ebf 
Signed-off-by: Daeyeon Jeong <daeyeon.dev@gmail.com>
joyeecheung
joyeecheung approved these changes Nov 7, 2022
View reviewed changes
Copy link
Member

@joyeecheung joyeecheung left a comment

Choose a reason for hiding this comment

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

Thanks for. the patience!

@daeyeon daeyeon added the request-ci Add this label to start a Jenkins CI on a PR. label Nov 9, 2022
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Nov 9, 2022
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@nodejs-github-bot
Copy link
Collaborator

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

@daeyeon
Copy link
Member Author

daeyeon commented Nov 9, 2022

Fixed the CI failure. PTAL again. Thanks!

@nodejs-github-bot
Copy link
Collaborator

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

joyeecheung
joyeecheung approved these changes Nov 9, 2022
View reviewed changes
Copy link
Member

@joyeecheung joyeecheung left a comment

Choose a reason for hiding this comment

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

Still LGTM, thanks

@nodejs-github-bot
Copy link
Collaborator

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

@daeyeon daeyeon added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Nov 9, 2022
@github-actions github-actions bot mentioned this pull request Nov 10, 2022
Open
20 tasks
@daeyeon daeyeon added the commit-queue Add this label to land a pull request using GitHub Actions. label Nov 10, 2022
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Nov 10, 2022
@nodejs-github-bot nodejs-github-bot merged commit 7b1e153 into nodejs:main Nov 10, 2022
@nodejs-github-bot
Copy link
Collaborator

Landed in 7b1e153

@daeyeon daeyeon deleted the main.exitcode-with-accessors-221022.Sat.fefa branch November 10, 2022 01:11
@RafaelGSS
Copy link
Member

Hi @daeyeon. I tried to include this commit to the v19.1.0 proposal, but I got some conflicts. Could you please open a backport PR?

@github-actions github-actions bot mentioned this pull request Nov 11, 2022
Open
19 tasks
@daeyeon
Copy link
Member Author

daeyeon commented Nov 11, 2022

Hi @RafaelGSS. Sure, I will open it.

@daeyeon
Copy link
Member Author

daeyeon commented Nov 11, 2022

@RafaelGSS Come to think of it, this requires 2d0d997. Since it's a semver-major change, I think it's right not to land this PR to v19.x.

This was referenced Nov 12, 2022
Open
Open
@daeyeon daeyeon added dont-land-on-v14.x dont-land-on-v18.x PRs that should not land on the v18.x-staging branch and should not be released in v18.x. and removed backport-requested-v19.x labels Nov 29, 2022
@targos targos mentioned this pull request Nov 29, 2022
Closed
@RafaelGSS RafaelGSS mentioned this pull request Apr 3, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@addaleax addaleax addaleax left review comments

@RaisinTen RaisinTen RaisinTen approved these changes

@joyeecheung joyeecheung joyeecheung approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. 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. dont-land-on-v18.x PRs that should not land on the v18.x-staging branch and should not be released in v18.x. lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. process Issues and PRs related to the process subsystem.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@daeyeon @nodejs-github-bot @RafaelGSS @addaleax @joyeecheung @RaisinTen