nodejs
src: migrate `String::Value` to `String::ValueView`
Fixes #54417 Ref: #55452
v8::String::Value is deprecated, so this PR replaces it with v8::String::ValueView
I've added https://github.com/nodejs/node/labels/needs-benchmark-ci in case this slows / speeds things up / down
I've also added https://github.com/nodejs/node/labels/needs-citgm 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.
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 169 commits behind head on main.
| Files with missing lines | Patch % | Lines |
|---|---|---|
| src/string_bytes.cc | 20.00% | 8 Missing :warning: |
| src/node_buffer.cc | 91.66% | 0 Missing and 1 partial :warning: |
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%) |
:arrow_down: |
| src/node_buffer.cc | 70.47% <91.66%> (+0.11%) |
:arrow_up: |
| src/string_bytes.cc | 68.89% <20.00%> (+0.45%) |
:arrow_up: |
![codecov[bot] avatar](https://avatars.githubusercontent.com/in/254?v=4 "Published by a pub.dev verified publisher"){kind=small}
I'm gonna hold off on a CI until the GitHub builds pass, because there might be failures that are already evident
Hey, can someone start a benchmark CI and CITGM?
The sooner these are done the sooner this can land :-)
RedYetiDev removed the https://github.com/nodejs/node/labels/needs-ci 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 https://github.com/nodejs/node/labels/needs-benchmark-ci in case this slows / speeds things up / down
What benchmark would we run?
Please don't do that, the label is there to inform the bot this PR requires passing Jenkins CI to land
That was an accident 😅, I don't even remember doing that.
What benchmark would we run?
I was thinking Buffer (for the indexOf), and string_bytes (if we have one)
CITGM (main): https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/3492/
CITGM (this PR): https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/3493/
Results:
FAILURE: 28 failures in 3493 not present in 3492
┌────────────────────────┬───────────────────┬──────────────────────────┬────────────────────────┬────────────────────────┬────────────────┬───────────────┬───────────────────┬────────────────────┬───────────────┬───────────────────────┬──────────────────────┬──────────────────┐
│ (index) │ 0 │ 1 │ 2 │ 3 │ 4 │ 5 │ 6 │ 7 │ 8 │ 9 │ 10 │ 11 │
├────────────────────────┼───────────────────┼──────────────────────────┼────────────────────────┼────────────────────────┼────────────────┼───────────────┼───────────────────┼────────────────────┼───────────────┼───────────────────────┼──────────────────────┼──────────────────┤
│ osx11 │ 'mime-v4.0.4' │ │ │ │ │ │ │ │ │ │ │ │
│ debian12-x64 │ 'jest-v29.7.0' │ 'mime-v4.0.4' │ │ │ │ │ │ │ │ │ │ │
│ rhel8-s390x │ 'mime-v4.0.4' │ 'readable-stream-v4.5.2' │ │ │ │ │ │ │ │ │ │ │
│ rhel8-ppc64le │ 'mime-v4.0.4' │ │ │ │ │ │ │ │ │ │ │ │
│ osx11-x64 │ 'mime-v4.0.4' │ │ │ │ │ │ │ │ │ │ │ │
│ fedora-last-latest-x64 │ 'jest-v29.7.0' │ 'mime-v4.0.4' │ │ │ │ │ │ │ │ │ │ │
│ rhel8-x64 │ 'jest-v29.7.0' │ 'mime-v4.0.4' │ │ │ │ │ │ │ │ │ │ │
│ ubuntu2204-64 │ 'jest-v29.7.0' │ 'mime-v4.0.4' │ │ │ │ │ │ │ │ │ │ │
│ win-vs2022 │ 'resolve-v1.22.8' │ │ │ │ │ │ │ │ │ │ │ │
│ fedora-latest-x64 │ 'dicer-v0.3.1' │ 'duplexer2-v0.1.4' │ 'end-of-stream-v1.4.4' │ 'full-icu-test-v1.0.3' │ 'jest-v29.7.0' │ 'koa-v2.15.3' │ 'lodash-v4.17.21' │ 'microtime-v3.1.1' │ 'mime-v4.0.4' │ 'multer-v1.4.5-lts.1' │ 'serialport-v12.0.0' │ 'sqlite3-v5.1.7' │
│ alpine-latest-x64 │ 'jest-v29.7.0' │ 'mime-v4.0.4' │ │ │ │ │ │ │ │ │ │ │
│ aix72-ppc64 │ │ │ │ │ │ │ │ │ │ │ │ │
└────────────────────────┴───────────────────┴──────────────────────────┴────────────────────────┴────────────────────────┴────────────────┴───────────────┴───────────────────┴────────────────────┴───────────────┴───────────────────────┴──────────────────────┴──────────────────┘
All failures appear to be not related to this change, most are ENOTFOUND on github URLs.
Can someone start a benchmark CI, or remove the label if you don't think it's needed?
Commit Queue failed
- Loading data for nodejs/node/pull/55458 ✔ Done loading data for nodejs/node/pull/55458 ----------------------------------- PR info ------------------------------------ Title src: migrate `String::Value` to `String::ValueView` (#55458) Author Aviv Keller <[email protected]> (@RedYetiDev) Branch RedYetiDev:v8-string--value-deprecate -> nodejs:main Labels c++, lib / src, author ready, needs-ci, needs-citgm, commit-queue-squash Commits 5 - src: migrate `String::Value` to `String::ValueView` - fixup! src: migrate `String::Value` to `String::ValueView` - fixup! fixup! src: migrate `String::Value` to `String::ValueView` - fixup! fixup! fixup! src: migrate `String::Value` to `String::ValueView` - fixup! fixup! fixup! fixup! src: migrate `String::Value` to `String::… Committers 1 - RedYetiDev <[email protected]> PR-URL: https://github.com/nodejs/node/pull/55458 Refs: https://github.com/nodejs/node/issues/55452 Reviewed-By: Vladimir Morozov <[email protected]> Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: James M Snell <[email protected]> ------------------------------ Generated metadata ------------------------------ PR-URL: https://github.com/nodejs/node/pull/55458 Refs: https://github.com/nodejs/node/issues/55452 Reviewed-By: Vladimir Morozov <[email protected]> Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: James M Snell <[email protected]> -------------------------------------------------------------------------------- ⚠ Commits were pushed since the last approving review: ⚠ - fixup! fixup! fixup! fixup! src: migrate `String::Value` to `String::… ℹ This PR was created on Sat, 19 Oct 2024 16:27:52 GMT ✔ Approvals: 3 ✔ - Vladimir Morozov (@vmoroz): https://github.com/nodejs/node/pull/55458#pullrequestreview-2379682983 ✔ - Yagiz Nizipli (@anonrig) (TSC): https://github.com/nodejs/node/pull/55458#pullrequestreview-2379711729 ✔ - James M Snell (@jasnell) (TSC): https://github.com/nodejs/node/pull/55458#pullrequestreview-2380689360 ✔ Last GitHub CI successful ℹ Last Full PR CI on 2024-10-19T20:50:38Z: https://ci.nodejs.org/job/node-test-pull-request/63206/ ℹ Last CITGM CI on 2024-10-24T21:10:17Z: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/3493/ - Querying data for job/node-test-pull-request/3493/ ✔ Last Jenkins CI successful -------------------------------------------------------------------------------- ✔ Aborted `git node land` session in /home/runner/work/node/node/.ncuhttps://github.com/nodejs/node/actions/runs/11543590208
