node icon indicating copy to clipboard operation
node copied to clipboard

src: remove icu based `ToASCII` and `ToUnicode`

Open anonrig opened this issue 1 year ago • 1 comments

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.

anonrig avatar Sep 28 '24 18:09 anonrig

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 88.41%. Comparing base (0c68991) to head (41641f7). Report is 8 commits behind head on main.

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     
Files with missing lines Coverage Δ
src/node_i18n.cc 73.74% <ø> (+0.67%) :arrow_up:
src/node_i18n.h 81.25% <ø> (ø)

... and 43 files with indirect coverage changes

codecov[bot] avatar Sep 28 '24 19:09 codecov[bot]

cc @nodejs/cpp-reviewers can you review?

anonrig avatar Oct 07 '24 22:10 anonrig

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

nodejs-github-bot avatar Oct 08 '24 00:10 nodejs-github-bot

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

nodejs-github-bot avatar Oct 10 '24 20:10 nodejs-github-bot

This appears to have broken the build when Node.js is configured with --with-intl=small-icu. e.g. https://ci.nodejs.org/job/node-test-commit-linux-containered/46896/nodes=ubuntu2204_sharedlibs_smallicu_x64/console

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 1

which is coming from the #ifdef NODE_HAVE_SMALL_ICU block in https://github.com/nodejs/node/blob/8e4ec9f3a67460d3094e5a7dfcef5f9493074ac8/src/node_i18n.cc#L550-L556

richardlau avatar Oct 11 '24 01:10 richardlau

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

nodejs-github-bot avatar Oct 18 '24 00:10 nodejs-github-bot

Landed in 9f5000e0f2a2c31d1f82999c70644ec1e9b2bf29

nodejs-github-bot avatar Oct 18 '24 16:10 nodejs-github-bot