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: use v8::Boolean(b) over b ? True() : False() #47554

Merged
merged 1 commit into from
Apr 16, 2023
Merged

src: use v8::Boolean(b) over b ? True() : False() #47554

merged 1 commit into from
Apr 16, 2023

Conversation

tniessen
Copy link
Member

Simplify existing code by using v8::Boolean::New() instead of equivalent expressions that contain ternary operators.

@tniessen
src: use v8::Boolean(b) over b ? True() : False()
e1f29eb 
Simplify existing code by using v8::Boolean::New() instead of equivalent
expressions that contain ternary operators.
@tniessen tniessen added c++ Issues and PRs that require attention from people who are familiar with C++. v8 engine Issues and PRs related to the V8 dependency. 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 Apr 13, 2023
@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/crypto
  • @nodejs/http2
  • @nodejs/net

@aduh95 aduh95 added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. labels Apr 14, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Apr 14, 2023
@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot

This comment was marked as off-topic.

Sign in to view
@nodejs-github-bot

This comment was marked as off-topic.

Sign in to view
@nodejs-github-bot

This comment was marked as off-topic.

Sign in to view
@tniessen tniessen removed the v8 engine Issues and PRs related to the V8 dependency. label Apr 14, 2023
@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
bnoordhuis
bnoordhuis approved these changes Apr 14, 2023
View reviewed changes
Copy link
Member

@bnoordhuis bnoordhuis left a comment

Choose a reason for hiding this comment

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

For background: True() and False() were at one time faster than Boolean::New() but that's no longer the case (it's literally implemented as value ? True() : False() now.)

@github-actions github-actions bot mentioned this pull request Apr 15, 2023
Open
33 tasks
@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/51303/

@tniessen tniessen added the commit-queue Add this label to land a pull request using GitHub Actions. label Apr 16, 2023
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Apr 16, 2023
@nodejs-github-bot nodejs-github-bot merged commit 4f5bb1a into nodejs:main Apr 16, 2023
@nodejs-github-bot
Copy link
Collaborator

Landed in 4f5bb1a

@github-actions github-actions bot mentioned this pull request Apr 18, 2023
Open
35 tasks
targos pushed a commit that referenced this pull request May 2, 2023
@tniessen @targos
src: use v8::Boolean(b) over b ? True() : False()
cf48db0 
Simplify existing code by using v8::Boolean::New() instead of equivalent
expressions that contain ternary operators.

PR-URL: #47554
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Jiawen Geng <[email protected]>
Reviewed-By: Mohammed Keyvanzadeh <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>
@targos targos mentioned this pull request May 2, 2023
Merged
danielleadams pushed a commit that referenced this pull request Jul 6, 2023
@tniessen @danielleadams
src: use v8::Boolean(b) over b ? True() : False()
c537fb8 
Simplify existing code by using v8::Boolean::New() instead of equivalent
expressions that contain ternary operators.

PR-URL: #47554
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Jiawen Geng <[email protected]>
Reviewed-By: Mohammed Keyvanzadeh <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>
MoLow pushed a commit to MoLow/node that referenced this pull request Jul 6, 2023
@tniessen @MoLow
src: use v8::Boolean(b) over b ? True() : False()
71fb476 
Simplify existing code by using v8::Boolean::New() instead of equivalent
expressions that contain ternary operators.

PR-URL: nodejs#47554
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Jiawen Geng <[email protected]>
Reviewed-By: Mohammed Keyvanzadeh <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>
@danielleadams danielleadams mentioned this pull request Jul 10, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@VoltrexKeyva VoltrexKeyva VoltrexKeyva approved these changes

@RaisinTen RaisinTen RaisinTen approved these changes

@gengjiawen gengjiawen gengjiawen approved these changes

@cjihrig cjihrig cjihrig approved these changes

@lpinca lpinca lpinca approved these changes

@jasnell jasnell jasnell approved these changes

@bnoordhuis bnoordhuis bnoordhuis 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++. lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
10 participants
@tniessen @nodejs-github-bot @bnoordhuis @jasnell @lpinca @cjihrig @gengjiawen @RaisinTen @VoltrexKeyva @aduh95