node
node copied to clipboard
nodejs
src: replace splitstring with built-in
Replaces SplitString function with built-in implementation available for C++20
Codecov Report
Attention: Patch coverage is 87.50000% with 1 line in your changes missing coverage. Please review.
Project coverage is 88.08%. Comparing base (
3ac5b49) to head (d9790fe). Report is 1099 commits behind head on main.
| Files with missing lines | Patch % | Lines |
|---|---|---|
| src/node_options.cc | 75.00% | 1 Missing :warning: |
Additional details and impacted files
@@ Coverage Diff @@
## main #54990 +/- ##
=======================================
Coverage 88.07% 88.08%
=======================================
Files 652 652
Lines 183653 183705 +52
Branches 35856 35865 +9
=======================================
+ Hits 161753 161809 +56
+ Misses 15146 15142 -4
Partials 6754 6754
| Files with missing lines | Coverage Ξ | |
|---|---|---|
| src/node_v8_platform-inl.h | 87.17% <100.00%> (+0.16%) |
:arrow_up: |
| src/util.cc | 87.41% <ΓΈ> (+0.07%) |
:arrow_up: |
| src/util.h | 89.91% <ΓΈ> (ΓΈ) |
|
| src/node_options.cc | 88.08% <75.00%> (+0.01%) |
:arrow_up: |
![codecov[bot] avatar](https://avatars.githubusercontent.com/in/254?v=4 "Published by a pub.dev verified publisher"){kind=small}
cc @nodejs/platform-smartos it seems smartos doesn't implement C++20 correctly.
@anonrig The log indicates that GCC 10 is used (which is the default in SmartOS as far as I can tell).
However, Node requires g++ 12.2.0 or clang++ 8.0.0 or better.
And, indeed, look carefully at the build log...
07:30:41 WARNING: C++ compiler (CXX=ccache g++, 10.3.0) too old, need g++ 12.2.0 or clang++ 8.0.0
It is surprising that this PR hits this issue.
SmartOS is in the process of being migrated to newer SmartOS as part of the migration of non-ARM servers from Equinix Metal: https://github.com/nodejs/build/issues/3731
https://github.com/nodejs/build/issues/3806 tracks gcc 12 deployment -- SmartOS is the remaining platform and the expectation is that moving to SmartOS 23 should resolve that.
@richardlau did the smartos migration complete? is there any blocker for this pull-request atm?
@richardlau did the smartos migration complete? is there any blocker for this pull-request atm?
@anonrig I'm not actively involved in the smartos migration, but I don't believe it's complete yet.
@jasnell @nodejs/platform-macos @nodejs/build this is blocked by the OSX infrastructure.
Commit Queue failed
- Loading data for nodejs/node/pull/54990 β Done loading data for nodejs/node/pull/54990 ----------------------------------- PR info ------------------------------------ Title src: replace splitstring with built-in (#54990) β Could not retrieve the email or name of the PR author's from user's GitHub profile! Branch anonrig:replace-stringviewsplit -> nodejs:main Labels c++, lib / src, needs-ci, dont-land-on-v18.x, dont-land-on-v20.x Commits 1 - src: replace splitstring with built-in Committers 1 - Yagiz Nizipli <[email protected]> PR-URL: https://github.com/nodejs/node/pull/54990 Reviewed-By: Daniel Lemire <[email protected]> Reviewed-By: Chengzhong Wu <[email protected]> Reviewed-By: Robert Nagy <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Matteo Collina <[email protected]> ------------------------------ Generated metadata ------------------------------ PR-URL: https://github.com/nodejs/node/pull/54990 Reviewed-By: Daniel Lemire <[email protected]> Reviewed-By: Chengzhong Wu <[email protected]> Reviewed-By: Robert Nagy <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Matteo Collina <[email protected]> -------------------------------------------------------------------------------- βΉ This PR was created on Wed, 18 Sep 2024 01:10:06 GMT β Approvals: 5 β - Daniel Lemire (@lemire): https://github.com/nodejs/node/pull/54990#pullrequestreview-2312621536 β - Chengzhong Wu (@legendecas) (TSC): https://github.com/nodejs/node/pull/54990#pullrequestreview-2317889526 β - Robert Nagy (@ronag) (TSC): https://github.com/nodejs/node/pull/54990#pullrequestreview-2317895154 β - James M Snell (@jasnell) (TSC): https://github.com/nodejs/node/pull/54990#pullrequestreview-2319866121 β - Matteo Collina (@mcollina) (TSC): https://github.com/nodejs/node/pull/54990#pullrequestreview-2572079210 β Last GitHub CI failed βΉ Last Full PR CI on 2025-02-09T18:11:51Z: https://ci.nodejs.org/job/node-test-pull-request/65094/ - Querying data for job/node-test-pull-request/65094/ β Last Jenkins CI successful -------------------------------------------------------------------------------- β Aborted `git node land` session in /home/runner/work/node/node/.ncuhttps://github.com/nodejs/node/actions/runs/13228903356
β Last GitHub CI failed
The GitHub UI is showing wrong labels. Jenkins CI passes. What is the proper action in this step? @nodejs/build
The only way to remove this check would be to rebase. The job was removed as part of the macOS update to 13
The only way to remove this check would be to rebase. The job was removed as part of the macOS update to 13
Would it be appropriate to land this manually?
That's what I would do
Cc @nodejs/tsc due to the removed ci job, I have to land this manually (to avoid re-running the whole CI one more time). Please comment here if you disagree. I'll land this tomorrow if not.
Landed in https://github.com/nodejs/node/commit/b0c6e10c5e01f4aec1035015dcd26fedb478571a
