Skip to content

src: add a variant of ToV8Value() #57576

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.

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

Closed
wants to merge 1 commit into from

Conversation

Aditi-1400
Copy link
Contributor

@Aditi-1400 Aditi-1400 commented Mar 21, 2025

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
Copy link

codecov bot commented Mar 21, 2025

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.

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.

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.

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

@joyeecheung
Copy link
Member

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

@nodejs-github-bot

This comment was marked as outdated.

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

@nodejs-github-bot

This comment was marked as outdated.

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

@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

jasnell pushed a commit that referenced this pull request May 12, 2025
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
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>
aduh95 pushed a commit that referenced this pull request Jun 10, 2025
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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
Development

Successfully merging this pull request may close these issues.

6 participants