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

v8: unnecessary call to icu::toUCharPtr requires ICU 58+ #19656

Closed
srl295 opened this issue Mar 28, 2018 · 9 comments
Closed

v8: unnecessary call to icu::toUCharPtr requires ICU 58+ #19656

srl295 opened this issue Mar 28, 2018 · 9 comments
Labels
i18n-api Issues and PRs related to the i18n implementation.

Comments

@srl295
Copy link
Member

srl295 commented Mar 28, 2018

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.
@srl295 srl295 added the i18n-api Issues and PRs related to the i18n implementation. label Mar 28, 2018
@srl295 srl295 changed the title v8: unnecessary call to icu::toUCharPtr blocks ICU v8: unnecessary call to icu::toUCharPtr requires ICU 58+ Mar 28, 2018
This was referenced Mar 28, 2018
Closed
Closed
@srl295
Copy link
Member Author

srl295 commented Mar 28, 2018 •
edited
Loading

Might be worked around by including the backported icu::toUCharPtr() function. edit yes, that seems to work. Will open a PR.

@srl295 srl295 mentioned this issue Mar 28, 2018
Closed
4 tasks
srl295 added a commit to srl295/node that referenced this issue Mar 28, 2018
@srl295
deps: backport char16ptr.h to 57
c53a1e2 
- char16ptr.h is appended to the ICU 57 unistr.h
- this way, no v8 code change is needed.

Fixes: nodejs#19656
@bnoordhuis
Copy link
Member

From your description of the issue, it sounds like V8 is the right place to fix this?

@srl295
Copy link
Member Author

srl295 commented Mar 28, 2018 •
edited
Loading

@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
Copy link
Member

bnoordhuis commented Mar 28, 2018 •
edited
Loading

I can look into (1) probably tomorrow.

@bnoordhuis
Copy link
Member

https://chromium-review.googlesource.com/c/v8/v8/+/987953

kisg pushed a commit to paul99/v8mips that referenced this issue Mar 31, 2018
@bnoordhuis
[intl] unbreak build with ICU 57
b767cde 
Remove a call to `icu::toUCharPtr()` that wasn't present in other
similar looking call sites either, just reinterpret_cast directly.

Fixes nodejs/node#19656.

Cq-Include-Trybots: luci.v8.try:v8_linux_noi18n_rel_ng
Change-Id: If281ce0a39356aa8bd20efb24c3e4b52b06841a3
Reviewed-on: https://chromium-review.googlesource.com/987953
Reviewed-by: Daniel Ehrenberg <littledan@chromium.org>
Commit-Queue: Ben Noordhuis <info@bnoordhuis.nl>
Cr-Commit-Position: refs/heads/master@{#52311}
bnoordhuis added a commit to bnoordhuis/io.js that referenced this issue Mar 31, 2018
@bnoordhuis
deps: cherry-pick b767cde1e7 from upstream V8
772393c 
Original commit message:

    [intl] unbreak build with ICU 57

    Remove a call to `icu::toUCharPtr()` that wasn't present in other
    similar looking call sites either, just reinterpret_cast directly.

    Fixes nodejs#19656.

    Cq-Include-Trybots: luci.v8.try:v8_linux_noi18n_rel_ng
    Change-Id: If281ce0a39356aa8bd20efb24c3e4b52b06841a3
    Reviewed-on: https://chromium-review.googlesource.com/987953
    Reviewed-by: Daniel Ehrenberg <littledan@chromium.org>
    Commit-Queue: Ben Noordhuis <info@bnoordhuis.nl>
    Cr-Commit-Position: refs/heads/master@{nodejs#52311}

Fixes: nodejs#19656
@bnoordhuis bnoordhuis mentioned this issue Mar 31, 2018
Closed
@bnoordhuis
Copy link
Member

#19710

@hashseed
Copy link
Member

hashseed commented 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
Copy link
Member

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
Copy link
Member Author

srl295 commented 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.

targos pushed a commit that referenced this issue Apr 12, 2018
@bnoordhuis @targos
deps: cherry-pick b767cde1e7 from upstream V8
41c8ab9 
Original commit message:

    [intl] unbreak build with ICU 57

    Remove a call to `icu::toUCharPtr()` that wasn't present in other
    similar looking call sites either, just reinterpret_cast directly.

    Fixes #19656.

    Cq-Include-Trybots: luci.v8.try:v8_linux_noi18n_rel_ng
    Change-Id: If281ce0a39356aa8bd20efb24c3e4b52b06841a3
    Reviewed-on: https://chromium-review.googlesource.com/987953
    Reviewed-by: Daniel Ehrenberg <littledan@chromium.org>
    Commit-Queue: Ben Noordhuis <info@bnoordhuis.nl>
    Cr-Commit-Position: refs/heads/master@{#52311}

PR-URL: #19710
Fixes: #19656
Refs: v8/v8@b767cde
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Steven R Loomis <srloomis@us.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos pushed a commit to targos/node that referenced this issue Apr 12, 2018
@bnoordhuis @targos
deps: cherry-pick b767cde1e7 from upstream V8
602ea95 
Original commit message:

    [intl] unbreak build with ICU 57

    Remove a call to `icu::toUCharPtr()` that wasn't present in other
    similar looking call sites either, just reinterpret_cast directly.

    Fixes nodejs#19656.

    Cq-Include-Trybots: luci.v8.try:v8_linux_noi18n_rel_ng
    Change-Id: If281ce0a39356aa8bd20efb24c3e4b52b06841a3
    Reviewed-on: https://chromium-review.googlesource.com/987953
    Reviewed-by: Daniel Ehrenberg <littledan@chromium.org>
    Commit-Queue: Ben Noordhuis <info@bnoordhuis.nl>
    Cr-Commit-Position: refs/heads/master@{nodejs#52311}

PR-URL: nodejs#19710
Fixes: nodejs#19656
Refs: v8/v8@b767cde
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Steven R Loomis <srloomis@us.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
jasnell pushed a commit that referenced this issue Apr 14, 2018
@bnoordhuis @jasnell
deps: cherry-pick b767cde1e7 from upstream V8
45f6dcc 
Original commit message:

    [intl] unbreak build with ICU 57

    Remove a call to `icu::toUCharPtr()` that wasn't present in other
    similar looking call sites either, just reinterpret_cast directly.

    Fixes #19656.

    Cq-Include-Trybots: luci.v8.try:v8_linux_noi18n_rel_ng
    Change-Id: If281ce0a39356aa8bd20efb24c3e4b52b06841a3
    Reviewed-on: https://chromium-review.googlesource.com/987953
    Reviewed-by: Daniel Ehrenberg <littledan@chromium.org>
    Commit-Queue: Ben Noordhuis <info@bnoordhuis.nl>
    Cr-Commit-Position: refs/heads/master@{#52311}

PR-URL: #19710
Fixes: #19656
Refs: v8/v8@b767cde
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Steven R Loomis <srloomis@us.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

PR-URL: #19980
Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>
Reviewed-By: Yang Guo <yangguo@chromium.org>
jasnell pushed a commit that referenced this issue Apr 16, 2018
@bnoordhuis @jasnell
deps: cherry-pick b767cde1e7 from upstream V8
b6aec1d 
Original commit message:

    [intl] unbreak build with ICU 57

    Remove a call to `icu::toUCharPtr()` that wasn't present in other
    similar looking call sites either, just reinterpret_cast directly.

    Fixes #19656.

    Cq-Include-Trybots: luci.v8.try:v8_linux_noi18n_rel_ng
    Change-Id: If281ce0a39356aa8bd20efb24c3e4b52b06841a3
    Reviewed-on: https://chromium-review.googlesource.com/987953
    Reviewed-by: Daniel Ehrenberg <littledan@chromium.org>
    Commit-Queue: Ben Noordhuis <info@bnoordhuis.nl>
    Cr-Commit-Position: refs/heads/master@{#52311}

PR-URL: #19710
Fixes: #19656
Refs: v8/v8@b767cde
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Steven R Loomis <srloomis@us.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

PR-URL: #19980
Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>
Reviewed-By: Yang Guo <yangguo@chromium.org>
MylesBorins pushed a commit that referenced this issue Aug 7, 2018
@bnoordhuis @MylesBorins
deps: cherry-pick b767cde1e7 from upstream V8
7ec216e 
Original commit message:

    [intl] unbreak build with ICU 57

    Remove a call to `icu::toUCharPtr()` that wasn't present in other
    similar looking call sites either, just reinterpret_cast directly.

    Fixes #19656.

    Cq-Include-Trybots: luci.v8.try:v8_linux_noi18n_rel_ng
    Change-Id: If281ce0a39356aa8bd20efb24c3e4b52b06841a3
    Reviewed-on: https://chromium-review.googlesource.com/987953
    Reviewed-by: Daniel Ehrenberg <littledan@chromium.org>
    Commit-Queue: Ben Noordhuis <info@bnoordhuis.nl>
    Cr-Commit-Position: refs/heads/master@{#52311}

PR-URL: #19710
Fixes: #19656
Refs: v8/v8@b767cde
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Steven R Loomis <srloomis@us.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this issue Aug 9, 2018
@bnoordhuis @MylesBorins
deps: cherry-pick b767cde1e7 from upstream V8
3049d78 
Original commit message:

    [intl] unbreak build with ICU 57

    Remove a call to `icu::toUCharPtr()` that wasn't present in other
    similar looking call sites either, just reinterpret_cast directly.

    Fixes #19656.

    Cq-Include-Trybots: luci.v8.try:v8_linux_noi18n_rel_ng
    Change-Id: If281ce0a39356aa8bd20efb24c3e4b52b06841a3
    Reviewed-on: https://chromium-review.googlesource.com/987953
    Reviewed-by: Daniel Ehrenberg <littledan@chromium.org>
    Commit-Queue: Ben Noordhuis <info@bnoordhuis.nl>
    Cr-Commit-Position: refs/heads/master@{#52311}

PR-URL: #19710
Fixes: #19656
Refs: v8/v8@b767cde
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Steven R Loomis <srloomis@us.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this issue Aug 16, 2018
@bnoordhuis @MylesBorins
deps: cherry-pick b767cde1e7 from upstream V8
d9ab189 
Original commit message:

    [intl] unbreak build with ICU 57

    Remove a call to `icu::toUCharPtr()` that wasn't present in other
    similar looking call sites either, just reinterpret_cast directly.

    Fixes #19656.

    Cq-Include-Trybots: luci.v8.try:v8_linux_noi18n_rel_ng
    Change-Id: If281ce0a39356aa8bd20efb24c3e4b52b06841a3
    Reviewed-on: https://chromium-review.googlesource.com/987953
    Reviewed-by: Daniel Ehrenberg <littledan@chromium.org>
    Commit-Queue: Ben Noordhuis <info@bnoordhuis.nl>
    Cr-Commit-Position: refs/heads/master@{#52311}

PR-URL: #19710
Fixes: #19656
Refs: v8/v8@b767cde
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Steven R Loomis <srloomis@us.ibm.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
Assignees
No one assigned
Labels
i18n-api Issues and PRs related to the i18n implementation.
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

deps: backport char16ptr.h to 57 srl295/node
3 participants
@bnoordhuis @srl295 @hashseed