Repository navigation
v8: unnecessary call to icu::toUCharPtr requires ICU 58+ #19656
Description
Split off from #19151
I found an issue in v8 which I worked around at srl295@384ab7d - introduced into v8 at v8/v8@99e8963 (and into node later) - there is a call to icu::toUCharPtr() passed as an input to a reinterpret_cast. This seems to unnecessarily tie the code to ICU 59+, and in any event ICU uses char16_t and not UChar as the type for C++ going forward. So toUCharPtr() is not something to call going forward. Background here
If this were fixed, ICU4C 57 ought to be supported by master at this point.
Activity
[-]v8: unnecessary call to icu::toUCharPtr blocks ICU [/-][+]v8: unnecessary call to icu::toUCharPtr requires ICU 58+ [/+]srl295 commentedon Mar 28, 2018
Might be worked around by including the backported
icu::toUCharPtr()function. edit yes, that seems to work. Will open a PR.bnoordhuis commentedon Mar 28, 2018
From your description of the issue, it sounds like V8 is the right place to fix this?
srl295 commentedon Mar 28, 2018
@bnoordhuis I now think so, per #19658 (comment)
for v8, what is needed here is either (1) fixing the call sites to stop using the unneeded casts or (2) including the backported char16ptr.h header. Both of these are small changes.
bnoordhuis commentedon Mar 28, 2018
I can look into (1) probably tomorrow.
bnoordhuis commentedon Mar 30, 2018
https://chromium-review.googlesource.com/c/v8/v8/+/987953
[intl] unbreak build with ICU 57
deps: cherry-pick b767cde1e7 from upstream V8
bnoordhuis commentedon Mar 31, 2018
#19710
hashseed commentedon Apr 2, 2018
Is there any reason to use earlier ICU? I'm asking also for future reference. I don't think V8 is expected to work with arbitrary ICU versions, as that's not what we test.
bnoordhuis commentedon Apr 2, 2018
It's not but this specific patch makes it work again with the system ICU on recent Ubuntu releases. That seemed worthwhile enough for a one-liner.
srl295 commentedon Apr 2, 2018
@hashseed No there is not a reason to use an earlier ICU usually. But in this case it was a regression introduced by v8/v8@99e8963 which just had an unneeded cast - there's also no reason to restrict earlier ICUs (especially when they are in active use) for non-feature/bugfix code.
deps: cherry-pick b767cde1e7 from upstream V8
deps: cherry-pick b767cde1e7 from upstream V8
deps: cherry-pick b767cde1e7 from upstream V8
deps: cherry-pick b767cde1e7 from upstream V8
deps: cherry-pick b767cde1e7 from upstream V8
deps: cherry-pick b767cde1e7 from upstream V8
deps: cherry-pick b767cde1e7 from upstream V8
deps: cherry-pick b767cde1e7 from upstream V8