src: remove icu based ToASCII and ToUnicode
#55156
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
nodejs-github-bot
added
c++
anonrig
force-pushed
the
remove-unused-functions
branch
from
4e0a3da
to
a0a5680
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #55156 +/- ##
=======================================
Coverage 88.40% 88.41%
=======================================
Files 653 653
Lines 187597 187502 -95
Branches 36112 36097 -15
=======================================
- Hits 165850 165781 -69
+ Misses 14967 14961 -6
+ Partials 6780 6760 -20
|
anonrig
force-pushed
the
remove-unused-functions
branch
from
a0a5680
to
e59a0cc
Compare
aduh95
reviewed
Sep 29, 2024
anonrig
force-pushed
the
remove-unused-functions
branch
from
e59a0cc
to
8e4ec9f
Compare
|
cc @nodejs/cpp-reviewers can you review? |
jasnell
approved these changes
Oct 7, 2024
github-actions
bot
removed
the
request-ci
KhafraDev
approved these changes
Oct 8, 2024
|
This appears to have broken the build when Node.js is configured with 21:54:07 In file included from ../deps/icu-small/source/common/unicode/platform.h:25,
21:54:07 from ../deps/icu-small/source/common/unicode/ptypes.h:46,
21:54:07 from ../deps/icu-small/source/common/unicode/umachine.h:46,
21:54:07 from ../deps/icu-small/source/common/unicode/utypes.h:38,
21:54:07 from ../deps/icu-small/source/common/unicode/ucnv_err.h:88,
21:54:07 from ../deps/icu-small/source/common/unicode/ucnv.h:51,
21:54:07 from ../src/node_i18n.h:34,
21:54:07 from ../src/node_i18n.cc:43:
21:54:07 ../src/node_i18n.cc: In function 'bool node::i18n::InitializeICUDirectory(const std::string&, std::string*)':
21:54:07 ../deps/icu-small/source/common/unicode/urename.h:900:54: error: 'udata_setCommonData_75' was not declared in this scope; did you mean 'udata_setCommonData'?
21:54:07 900 | #define udata_setCommonData U_ICU_ENTRY_POINT_RENAME(udata_setCommonData)
21:54:07 | ^~~~~~~~~~~~~~~~~~~
21:54:07 ../deps/icu-small/source/common/unicode/uvernum.h:121:50: note: in definition of macro 'U_DEF_ICU_ENTRY_POINT_RENAME'
21:54:07 121 | # define U_DEF_ICU_ENTRY_POINT_RENAME(x,y) x ## y
21:54:07 | ^
21:54:07 ../deps/icu-small/source/common/unicode/uvernum.h:123:47: note: in expansion of macro 'U_DEF2_ICU_ENTRY_POINT_RENAME'
21:54:07 123 | # define U_ICU_ENTRY_POINT_RENAME(x) U_DEF2_ICU_ENTRY_POINT_RENAME(x,U_ICU_VERSION_SUFFIX)
21:54:07 | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
21:54:07 ../deps/icu-small/source/common/unicode/urename.h:900:29: note: in expansion of macro 'U_ICU_ENTRY_POINT_RENAME'
21:54:07 900 | #define udata_setCommonData U_ICU_ENTRY_POINT_RENAME(udata_setCommonData)
21:54:07 | ^~~~~~~~~~~~~~~~~~~~~~~~
21:54:07 ../src/node_i18n.cc:555:5: note: in expansion of macro 'udata_setCommonData'
21:54:07 555 | udata_setCommonData(&SMALL_ICUDATA_ENTRY_POINT, &status);
21:54:07 | ^~~~~~~~~~~~~~~~~~~
21:54:07 make[2]: *** [libnode.target.mk:527: /home/iojs/build/workspace/node-test-commit-linux-containered/out/Release/obj.target/libnode/src/node_i18n.o] Error 1which is coming from the Lines 550 to 556 in 8e4ec9f |
anonrig
force-pushed
the
remove-unused-functions
branch
from
dee0c58
to
41641f7
Compare
anonrig
added
the
commit-queue-squash
richardlau
added
request-ci
richardlau
approved these changes
Oct 18, 2024
anonrig
added
the
commit-queue
nodejs-github-bot
removed
the
commit-queue
|
Landed in 9f5000e |
aduh95
pushed a commit
that referenced
this pull request
Oct 19, 2024
PR-URL: #55156 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Matthew Aitken <maitken033380023@gmail.com> Reviewed-By: Daniel Lemire <daniel@lemire.me> Reviewed-By: Richard Lau <rlau@redhat.com>
Merged
louwers
pushed a commit
to louwers/node
that referenced
this pull request
Nov 2, 2024
PR-URL: nodejs#55156 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Matthew Aitken <maitken033380023@gmail.com> Reviewed-By: Daniel Lemire <daniel@lemire.me> Reviewed-By: Richard Lau <rlau@redhat.com>
tpoisseau
pushed a commit
to tpoisseau/node
that referenced
this pull request
Nov 21, 2024
PR-URL: nodejs#55156 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Matthew Aitken <maitken033380023@gmail.com> Reviewed-By: Daniel Lemire <daniel@lemire.me> Reviewed-By: Richard Lau <rlau@redhat.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
We have been using Ada's ToASCII and ToUnicode implementations since Node 18. I think it's safe to assume we can remove this "unused" code.