Skip to content

src: add FromV8Value<T>() for integral and enum types#57931

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
nodejs-github-bot merged 1 commit into from
Jun 24, 2025
Merged

src: add FromV8Value<T>() for integral and enum types #57931

nodejs-github-bot merged 1 commit into from
Jun 24, 2025

Conversation

@Aditi-1400
Copy link
Contributor

@Aditi-1400Aditi-1400 commented Apr 18, 2025•
edited
Loading

From the following comment in the pull request: #57146 (comment)

Add a template utility method FromV8Value<T>() to replace the repetitive static_cast<...>(value.As<>()‑>Value()) pattern. It also additionally adds compile‑time range checks for integers.

I've also replaced a bunch of static_casts with the new FromV8Value<T>()

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Apr 18, 2025

Review requested:

  • @nodejs/crypto
  • @nodejs/net

@nodejs-github-botnodejs-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 Apr 18, 2025
@codecov
Copy link

codecovbot commented Apr 18, 2025•
edited
Loading

Codecov Report

Attention: Patch coverage is 88.23529% with 2 lines in your changes missing coverage. Please review.

Project coverage is 90.27%. Comparing base (9cc0195) to head (fabec6a).
Report is 480 commits behind head on main.

Files with missing linesPatch %Lines
src/util-inl.h66.66%0 Missing and 2 partials ⚠️
Additional details and impacted files
@@ Coverage Diff @@## main #57931 +/- ## ========================================== - Coverage 90.28% 90.27% -0.01%  ========================================== Files 630 630 Lines 186158 186160 +2 Branches 36484 36476 -8 ========================================== - Hits 168067 168057 -10 + Misses 10974 10970 -4 - Partials 7117 7133 +16
Files with missing linesCoverage Δ
src/crypto/crypto_keys.cc72.57% <100.00%> (+0.27%)⬆️
src/node_file.cc76.76% <100.00%> (+0.04%)⬆️
src/node_util.cc81.73% <100.00%> (-0.06%)⬇️
src/node_v8.cc80.67% <100.00%> (-0.05%)⬇️
src/node_zlib.cc78.76% <100.00%> (+0.34%)⬆️
src/process_wrap.cc66.66% <100.00%> (ø)
src/udp_wrap.cc78.62% <100.00%> (ø)
src/util-inl.h80.68% <66.66%> (-0.30%)⬇️

... and 28 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

joyeecheung
joyeecheung previously approved these changes Apr 18, 2025
View reviewed changes
@joyeecheungjoyeecheung added the request-ci Add this label to start a Jenkins CI on a PR. label Apr 18, 2025
joyeecheung
joyeecheung reviewed Apr 18, 2025
View reviewed changes
Copy link
Member

@joyeecheungjoyeecheung left a comment

Choose a reason for hiding this comment

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

hmm, actually the CHECKs should be more specific..

@joyeecheungjoyeecheung dismissed their stale reviewApril 18, 2025 19:53

need more specific CHECKs

@github-actionsgithub-actionsbot added request-ci-failed An error occurred while starting CI via request-ci label, and manual interventon is needed. and removed request-ci Add this label to start a Jenkins CI on a PR. labels Apr 18, 2025
@github-actions
Copy link
Contributor

github-actionsbot commented Apr 18, 2025

Failed to start CI
 ⚠ No approving reviews found ✘ Refusing to run CI on potentially unsafe PR
https://github.com/nodejs/node/actions/runs/14540925539

@joyeecheungjoyeecheung added request-ci Add this label to start a Jenkins CI on a PR. and removed request-ci-failed An error occurred while starting CI via request-ci label, and manual interventon is needed. request-ci Add this label to start a Jenkins CI on a PR. labels Apr 23, 2025
@joyeecheung
Copy link
Member

joyeecheung commented Apr 23, 2025

This now has a conflict with the main branch, can you rebase?

@Aditi-1400
src: add FromV8Value<T>() for integral and enum types
fabec6a
Add a template utility method FromV8Value<T>() to replace the repetitive static_cast<...>(value.As<>()‑>Value()) pattern. It also additionally adds compile‑time range checks for integers. Refs: nodejs#57146 (comment)
@joyeecheungjoyeecheung added the request-ci Add this label to start a Jenkins CI on a PR. label May 2, 2025
@github-actionsgithub-actionsbot removed the request-ci Add this label to start a Jenkins CI on a PR. label May 2, 2025
@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

This comment was marked as outdated.

Sign in to view
@dani2542
Copy link

dani2542 commented May 3, 2025 via email

@joyeecheungjoyeecheung added the request-ci Add this label to start a Jenkins CI on a PR. label May 7, 2025
@aduh95aduh95 added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. and removed request-ci Add this label to start a Jenkins CI on a PR. labels May 7, 2025
@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

This comment was marked as outdated.

Sign in to view
@joyeecheungjoyeecheung added the request-ci Add this label to start a Jenkins CI on a PR. label May 26, 2025
@github-actionsgithub-actionsbot removed the request-ci Add this label to start a Jenkins CI on a PR. label May 26, 2025
@nodejs-github-bot

This comment was marked as outdated.

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

nodejs-github-bot commented May 26, 2025

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

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented May 27, 2025

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

@jasnelljasnell added the commit-queue Add this label to land a pull request using GitHub Actions. label Jun 24, 2025
@nodejs-github-botnodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Jun 24, 2025
@nodejs-github-botnodejs-github-bot merged commit faada65 into nodejs:mainJun 24, 2025
72 checks passed
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Jun 24, 2025

Landed in faada65

RafaelGSS pushed a commit that referenced this pull request Jun 24, 2025
@Aditi-1400@RafaelGSS
src: add FromV8Value<T>() for integral and enum types
8c20896
Add a template utility method FromV8Value<T>() to replace the repetitive static_cast<...>(value.As<>()‑>Value()) pattern. It also additionally adds compile‑time range checks for integers. Refs: #57146 (comment) PR-URL: #57931 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Joyee Cheung <[email protected]>
@github-actionsgithub-actionsbot mentioned this pull request Jun 24, 2025
Merged
@kt3kkt3k mentioned this pull request Jun 25, 2025
Closed
aduh95 pushed a commit that referenced this pull request Jul 21, 2025
@Aditi-1400@aduh95
src: add FromV8Value<T>() for integral and enum types
2798a63
Add a template utility method FromV8Value<T>() to replace the repetitive static_cast<...>(value.As<>()‑>Value()) pattern. It also additionally adds compile‑time range checks for integers. Refs: #57146 (comment) PR-URL: #57931 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Joyee Cheung <[email protected]>
aduh95 pushed a commit that referenced this pull request Jul 24, 2025
@Aditi-1400@aduh95
src: add FromV8Value<T>() for integral and enum types
74ef07f
Add a template utility method FromV8Value<T>() to replace the repetitive static_cast<...>(value.As<>()‑>Value()) pattern. It also additionally adds compile‑time range checks for integers. Refs: #57146 (comment) PR-URL: #57931 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Joyee Cheung <[email protected]>
@github-actionsgithub-actionsbot mentioned this pull request Jul 28, 2025
Merged
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jasnelljasnelljasnell approved these changes

@joyeecheungjoyeecheungjoyeecheung approved these changes

Assignees

No one assigned

Labels

author readyPRs 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 / srcIssues and PRs related to general changes in the lib or src directory.needs-ciPRs that need a full CI run.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@Aditi-1400@nodejs-github-bot@joyeecheung@dani2542@jasnell@aduh95