Skip to content

src: add a variant of ToV8Value()#57576

Closed
Aditi-1400 wants to merge 1 commit intonodejs:mainfrom
Aditi-1400:primitive-array
Closed

src: add a variant of ToV8Value()#57576
Aditi-1400 wants to merge 1 commit intonodejs:mainfrom
Aditi-1400:primitive-array

Conversation

@Aditi-1400
Copy link
Contributor

@Aditi-1400 Aditi-1400 commented Mar 21, 2025 •
edited
Loading

Adds a variant of ToV8Value for array of primitives that do not need to throw during conversion - there is essentially no exceptions that can be thrown then you create an array of integers and you are only dealing with the one-size-fit-all abstraction of ToV8Value.

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. labels Mar 21, 2025
@Aditi-1400 Aditi-1400 force-pushed the primitive-array branch 2 times, most recently from 565d8b1 to fc7eafe Compare March 21, 2025 14:09
jasnell
jasnell reviewed Mar 21, 2025
View reviewed changes
jasnell
jasnell reviewed Mar 21, 2025
View reviewed changes
jasnell
jasnell reviewed Mar 21, 2025
View reviewed changes
@codecov
Copy link

codecov bot commented Mar 21, 2025 •
edited
Loading

Codecov Report

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

Project coverage is 90.24%. Comparing base (922ce9d) to head (d037058).
Report is 411 commits behind head on main.

Files with missing lines Patch % Lines
src/util-inl.h 86.66% 0 Missing and 2 partials ⚠️
Additional details and impacted files 
@@           Coverage Diff            @@
##             main   #57576    +/-   ##
========================================
  Coverage   90.23%   90.24%            
========================================
  Files         629      630     +1     
  Lines      184939   185080   +141     
  Branches    36232    36229     -3     
========================================
+ Hits       166885   167020   +135     
- Misses      11011    11031    +20     
+ Partials     7043     7029    -14
Files with missing lines Coverage Δ
src/node_v8.cc 81.72% <100.00%> (+1.00%) ⬆️
src/util-inl.h 84.39% <86.66%> (+1.12%) ⬆️

... and 73 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.

anonrig
anonrig requested changes Mar 21, 2025
View reviewed changes
Copy link
Member

@anonrig anonrig left a comment

Choose a reason for hiding this comment

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

This is not a better replacement than what we have right now, and lacks real world usage. We shouldn't introduce new functions that we don't use at all.

@Aditi-1400
src: add a variant of ToV8Value() for primitive arrays
d037058 
Adds a variant of ToV8Value for array of primitives that
do not need to throw during conversion - there is essentially
no exceptions that can be thrown then an array
of integers is created.
@jasnell jasnell requested a review from anonrig April 5, 2025 15:36
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@joyeecheung joyeecheung added the request-ci Add this label to start a Jenkins CI on a PR. label Apr 10, 2025
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Apr 10, 2025
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@joyeecheung
Copy link
Member

joyeecheung commented Apr 16, 2025

@anonrig Are you still blocking the PR? It's updated significantly and the helper is being used in a function.

@joyeecheung joyeecheung added the request-ci Add this label to start a Jenkins CI on a PR. label Apr 16, 2025
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Apr 16, 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
@joyeecheung joyeecheung added the request-ci Add this label to start a Jenkins CI on a PR. label May 2, 2025
@github-actions github-actions bot 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
@joyeecheung joyeecheung added the request-ci Add this label to start a Jenkins CI on a PR. label May 7, 2025
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label May 7, 2025
@nodejs-github-bot

This comment was marked as duplicate.

Sign in to view
@aduh95 aduh95 added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label May 7, 2025
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented May 12, 2025 •
edited by jasnell
Loading

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

jasnell pushed a commit that referenced this pull request May 12, 2025
@Aditi-1400 @jasnell
src: add a variant of ToV8Value() for primitive arrays
28cbc4c 
Adds a variant of ToV8Value for array of primitives that
do not need to throw during conversion - there is essentially
no exceptions that can be thrown then an array
of integers is created.

PR-URL: #57576
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
@jasnell
Copy link
Member

jasnell commented May 12, 2025

Landed in 28cbc4c

@jasnell jasnell closed this May 12, 2025
targos pushed a commit that referenced this pull request May 16, 2025
@Aditi-1400 @targos
src: add a variant of ToV8Value() for primitive arrays
e0355b7 
Adds a variant of ToV8Value for array of primitives that
do not need to throw during conversion - there is essentially
no exceptions that can be thrown then an array
of integers is created.

PR-URL: #57576
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
@github-actions github-actions bot mentioned this pull request May 20, 2025
Merged
aduh95 pushed a commit that referenced this pull request Jun 10, 2025
@Aditi-1400 @aduh95
src: add a variant of ToV8Value() for primitive arrays
aa00f59 
Adds a variant of ToV8Value for array of primitives that
do not need to throw during conversion - there is essentially
no exceptions that can be thrown then an array
of integers is created.

PR-URL: #57576
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
@github-actions github-actions bot mentioned this pull request Jun 10, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jasnell jasnell jasnell approved these changes

@anonrig anonrig anonrig 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++. 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.

6 participants

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