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: replace splitstring with built-in #54990

Merged
merged 1 commit into from
Feb 10, 2025
Merged

src: replace splitstring with built-in #54990

merged 1 commit into from
Feb 10, 2025
+11 −31

Conversation

anonrig
Copy link
Member

@anonrig anonrig commented Sep 18, 2024

Replaces SplitString function with built-in implementation available for C++20

@anonrig anonrig added dont-land-on-v18.x PRs that should not land on the v18.x-staging branch and should not be released in v18.x. dont-land-on-v20.x PRs that should not land on the v20.x-staging branch and should not be released in v20.x. labels Sep 18, 2024
@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 Sep 18, 2024
@codecov Codecov
Copy link

codecov bot commented Sep 18, 2024 •
edited
Loading

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 ⚠️
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%) ⬆️
src/util.cc 87.41% <ø> (+0.07%) ⬆️
src/util.h 89.91% <ø> (ø)
src/node_options.cc 88.08% <75.00%> (+0.01%) ⬆️

... and 41 files with indirect coverage changes

@anonrig anonrig requested a review from lemire September 18, 2024 02:29
@anonrig anonrig force-pushed the replace-stringviewsplit branch from 43335d5 to d9790fe Compare September 18, 2024 14:02
@anonrig
Copy link
Member Author

anonrig commented Sep 18, 2024

cc @nodejs/cpp-reviewers

@anonrig anonrig requested a review from ronag September 19, 2024 19:07
@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
@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
@anonrig
Copy link
Member Author

anonrig commented Sep 23, 2024

cc @nodejs/platform-smartos it seems smartos doesn't implement C++20 correctly.

@lemire
Copy link
Member

lemire commented Sep 24, 2024

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

@richardlau
Copy link
Member

SmartOS is in the process of being migrated to newer SmartOS as part of the migration of non-ARM servers from Equinix Metal: nodejs/build#3731

nodejs/build#3806 tracks gcc 12 deployment -- SmartOS is the remaining platform and the expectation is that moving to SmartOS 23 should resolve that.

@anonrig
Copy link
Member Author

anonrig commented Oct 24, 2024

@richardlau did the smartos migration complete? is there any blocker for this pull-request atm?

@richardlau
Copy link
Member

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

@anonrig anonrig added the blocked PRs that are blocked by other issues or PRs. label Dec 6, 2024
This was referenced Dec 11, 2024
Closed
Open
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@anonrig
Copy link
Member Author

anonrig commented Jan 24, 2025

@jasnell @nodejs/platform-macos @nodejs/build this is blocked by the OSX infrastructure.

@jasnell
Copy link
Member

jasnell commented Jan 24, 2025

@nodejs/tsc ... just fyi on another CI infrastructure block

mcollina
mcollina approved these changes Jan 24, 2025
View reviewed changes
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

@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
@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
@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
@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@anonrig anonrig added commit-queue Add this label to land a pull request using GitHub Actions. and removed blocked PRs that are blocked by other issues or PRs. labels Feb 9, 2025
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Feb 9, 2025
@nodejs-github-bot
Copy link
Collaborator

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 <yagiz@cloudflare.com>
PR-URL: https://github.com/nodejs/node/pull/54990
Reviewed-By: Daniel Lemire <daniel@lemire.me>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/54990
Reviewed-By: Daniel Lemire <daniel@lemire.me>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
--------------------------------------------------------------------------------
   ℹ  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/.ncu
https://github.com/nodejs/node/actions/runs/13228903356

@nodejs-github-bot nodejs-github-bot added the commit-queue-failed An error occurred while landing this pull request using GitHub Actions. label Feb 9, 2025
@anonrig
Copy link
Member Author

anonrig commented Feb 9, 2025

✘ Last GitHub CI failed

The GitHub UI is showing wrong labels. Jenkins CI passes. What is the proper action in this step? @nodejs/build

@targos
Copy link
Member

targos commented Feb 9, 2025

The only way to remove this check would be to rebase. The job was removed as part of the macOS update to 13

@anonrig
Copy link
Member Author

anonrig commented Feb 9, 2025

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?

@targos
Copy link
Member

targos commented Feb 9, 2025

That's what I would do

@anonrig
Copy link
Member Author

anonrig commented Feb 9, 2025

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.

@anonrig @aduh95
src: replace SplitString with built-in
b0c6e10 
PR-URL: nodejs#54990
Reviewed-By: Daniel Lemire <daniel@lemire.me>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
@aduh95 aduh95 force-pushed the replace-stringviewsplit branch from 6511d48 to b0c6e10 Compare February 10, 2025 17:51
@aduh95 aduh95 merged commit b0c6e10 into nodejs:main Feb 10, 2025
18 checks passed
@aduh95
Copy link
Contributor

aduh95 commented Feb 10, 2025

Landed in b0c6e10

targos pushed a commit that referenced this pull request Feb 11, 2025
@anonrig @targos
src: replace SplitString with built-in
6b398d6 
PR-URL: #54990
Reviewed-By: Daniel Lemire <daniel@lemire.me>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
@github-actions github-actions bot mentioned this pull request Feb 11, 2025
Open
@zcbenz zcbenz mentioned this pull request Feb 12, 2025
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lemire lemire lemire approved these changes

@mcollina mcollina mcollina approved these changes

@jasnell jasnell jasnell approved these changes

@ronag ronag ronag approved these changes

@legendecas legendecas legendecas approved these changes

Assignees
No one assigned
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. commit-queue-failed An error occurred while landing this pull request using GitHub Actions. dont-land-on-v18.x PRs that should not land on the v18.x-staging branch and should not be released in v18.x. dont-land-on-v20.x PRs that should not land on the v20.x-staging branch and should not be released in v20.x. lib / src Issues and PRs related to general changes in the lib or src directory. 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.
10 participants
@anonrig @nodejs-github-bot @lemire @richardlau @jasnell @targos @aduh95 @mcollina @ronag @legendecas