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: migrate String::Value to String::ValueView #55458

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

src: migrate String::Value to String::ValueView #55458

wants to merge 5 commits into from
+25 −28

Conversation

RedYetiDev
Copy link
Member

Fixes #54417
Ref: #55452

v8::String::Value is deprecated, so this PR replaces it with v8::String::ValueView

@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 19, 2024
@RedYetiDev RedYetiDev mentioned this pull request Oct 19, 2024
Open
7 tasks
@RedYetiDev RedYetiDev added needs-citgm PRs that need a CITGM CI run. needs-benchmark-ci PR that need a benchmark CI run. labels Oct 19, 2024
@RedYetiDev
Copy link
Member Author

I've added needs-benchmark-ci PR that need a benchmark CI run. in case this slows / speeds things up / down

I've also added needs-citgm PRs that need a CITGM CI run. as a "just-in-case", as this modifies very important code (E.G. Buffer's IndexOf), and it would be unfortunate if it breaks something we were unaware of.

@RedYetiDev RedYetiDev changed the title src:: migrate String::Value to String::ValueView src: migrate String::Value to String::ValueView Oct 19, 2024
anonrig
anonrig reviewed Oct 19, 2024
View reviewed changes
src/node_buffer.cc Outdated Show resolved Hide resolved
@codecov Codecov
Copy link

codecov bot commented Oct 19, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 62.50000% with 9 lines in your changes missing coverage. Please review.

Project coverage is 88.41%. Comparing base (8ed50bc) to head (862d75c).
Report is 17 commits behind head on main.

Files with missing lines Patch % Lines
src/string_bytes.cc 20.00% 8 Missing ⚠️
src/node_buffer.cc 91.66% 0 Missing and 1 partial ⚠️
Additional details and impacted files 
@@           Coverage Diff           @@
##             main   #55458   +/-   ##
=======================================
  Coverage   88.41%   88.41%           
=======================================
  Files         653      653           
  Lines      187485   187429   -56     
  Branches    36095    36070   -25     
=======================================
- Hits       165764   165717   -47     
+ Misses      14966    14950   -16     
- Partials     6755     6762    +7
Files with missing lines Coverage Δ
src/inspector_js_api.cc 86.24% <100.00%> (-0.06%) ⬇️
src/node_buffer.cc 70.47% <91.66%> (+0.11%) ⬆️
src/string_bytes.cc 68.89% <20.00%> (+0.45%) ⬆️

... and 39 files with indirect coverage changes

vmoroz
vmoroz reviewed Oct 19, 2024
View reviewed changes
src/inspector_js_api.cc Outdated Show resolved Hide resolved
vmoroz
vmoroz reviewed Oct 19, 2024
View reviewed changes
src/node_buffer.cc Outdated Show resolved Hide resolved
vmoroz
vmoroz reviewed Oct 19, 2024
View reviewed changes
src/node_buffer.cc Outdated Show resolved Hide resolved
vmoroz
vmoroz approved these changes Oct 19, 2024
View reviewed changes
Copy link
Member

@vmoroz vmoroz left a comment

Choose a reason for hiding this comment

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

LGTM

@RedYetiDev RedYetiDev added request-ci Add this label to start a Jenkins CI on a PR. author ready PRs that have at least one approval, no pending requests for changes, and a CI started. and removed 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 Oct 19, 2024
@RedYetiDev
Copy link
Member Author

I'm gonna hold off on a CI until the GitHub builds pass, because there might be failures that are already evident

@RedYetiDev RedYetiDev 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 Oct 19, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 19, 2024
@nodejs-github-bot
Copy link
Collaborator

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

@RedYetiDev RedYetiDev removed the needs-ci PRs that need a full CI run. label Oct 20, 2024
@RedYetiDev
Copy link
Member Author

RedYetiDev commented Oct 20, 2024 •
edited
Loading

Hey, can someone start a benchmark CI and CITGM?

The sooner these are done the sooner this can land :-)

addaleax
addaleax reviewed Oct 21, 2024
View reviewed changes
src/string_bytes.cc Outdated Show resolved Hide resolved
@RedYetiDev RedYetiDev added the needs-ci PRs that need a full CI run. label Oct 21, 2024
@aduh95
Copy link
Contributor

aduh95 commented Oct 22, 2024

RedYetiDev removed the needs-ci PRs that need a full CI run. label

Please don't do that, the label is there to inform the bot this PR requires passing Jenkins CI to land

I've added needs-benchmark-ci PR that need a benchmark CI run. in case this slows / speeds things up / down

What benchmark would we run?

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

@vmoroz vmoroz vmoroz approved these changes

@anonrig anonrig anonrig approved these changes

@addaleax addaleax Awaiting requested review from addaleax

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-benchmark-ci PR that need a benchmark CI run. needs-ci PRs that need a full CI run. needs-citgm PRs that need a CITGM CI run.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

v8::String::Value is deprecated
7 participants
@RedYetiDev @nodejs-github-bot @aduh95 @jasnell @addaleax @vmoroz @anonrig