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.

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

src: remove icu usage from node_string.cc #46548

Merged
merged 1 commit into from
Feb 17, 2023

Conversation

anonrig
Copy link
Member

@anonrig anonrig commented Feb 7, 2023

@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 Feb 7, 2023
@anonrig anonrig force-pushed the remove-icu-from-node-string branch 2 times, most recently from 7ed8398 to 870e211 Compare February 7, 2023 22:23
@targos
Copy link
Member

targos commented Feb 8, 2023

@nodejs/cpp-reviewers

Copy link
Member

@bnoordhuis bnoordhuis left a comment

Choose a reason for hiding this comment

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

LGTM with a suggestion.

src/inspector/node_string.cc Show resolved Hide resolved
Trott
Trott previously approved these changes Feb 8, 2023
Copy link
Member

@Trott Trott left a comment

Choose a reason for hiding this comment

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

LGTM with @bnoordhuis's suggestion

@Trott Trott dismissed their stale review February 8, 2023 16:59

I'm going to dismiss my review because it is a bit rubber-stamp-y. This change looks good to me, but my competence in this area is significantly less than that of others who will hopefully review.

@anonrig anonrig force-pushed the remove-icu-from-node-string branch from 870e211 to 210b57e Compare February 8, 2023 20:53
@anonrig
Copy link
Member Author

anonrig commented Feb 8, 2023

@bnoordhuis Can you review it again?

Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

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

LGTM

@anonrig anonrig added the request-ci Add this label to start a Jenkins CI on a PR. label Feb 17, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Feb 17, 2023
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@anonrig anonrig added the commit-queue Add this label to land a pull request using GitHub Actions. label Feb 17, 2023
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Feb 17, 2023
@nodejs-github-bot nodejs-github-bot merged commit 7796692 into nodejs:main Feb 17, 2023
@nodejs-github-bot
Copy link
Collaborator

Landed in 7796692

@anonrig anonrig deleted the remove-icu-from-node-string branch February 17, 2023 16:36
MylesBorins pushed a commit that referenced this pull request Feb 18, 2023
PR-URL: #46548
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Michael Dawson <midawson@redhat.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Feb 19, 2023
MylesBorins pushed a commit that referenced this pull request Feb 20, 2023
PR-URL: #46548
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Michael Dawson <midawson@redhat.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
StefanStojanovic added a commit to JaneaSystems/node that referenced this pull request Feb 22, 2023
Many tests started failing on ARM64 Windows after migrating from icu to
simdutf. This change reverts those changes for the problematic platform.

Refs: nodejs#46471
Refs: nodejs#46472
Refs: nodejs#46548
Refs: simdutf/simdutf#216
danielleadams pushed a commit that referenced this pull request Apr 11, 2023
PR-URL: #46548
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Michael Dawson <midawson@redhat.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

7 participants