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

deps: issues with v8 and prior ICUs #19151

Closed
srl295 opened this issue Mar 5, 2018 · 1 comment
Closed

deps: issues with v8 and prior ICUs #19151

srl295 opened this issue Mar 5, 2018 · 1 comment
Labels
i18n-api Issues and PRs related to the i18n implementation.

Comments

@srl295
Copy link
Member

srl295 commented Mar 5, 2018

  • Version: master
  • Platform: mac
  • Subsystem: deps

configure --with-icu-source=https://ssl.icu-project.org/files/icu4c/58.2/icu4c-58_2-src.zip --with-intl=full-icu

… (while bypassing #19150 by avoiding iculslocs ) will exhibit the following error:

../../deps/v8/src/objects/intl-objects.cc:733:19: error: no member named 'toUTF8String' in 'icu_58::UnicodeString'
        category->toUTF8String(keyword).data());
        ~~~~~~~~  ^
1 error generated.

The above is probably due to std::string now being used directly by v8 with ICU, and this might fix it (note a comment that's now wrong).

diff --git a/tools/icu/icu-generic.gyp b/tools/icu/icu-generic.gyp
index 8f29369255..a50f12d794 100644
--- a/tools/icu/icu-generic.gyp
+++ b/tools/icu/icu-generic.gyp
@@ -35,9 +35,7 @@
           'UCONFIG_NO_REGULAR_EXPRESSIONS=1',
           'U_ENABLE_DYLOAD=0',
           'U_STATIC_IMPLEMENTATION=1',
-          # Don't need std::string in API.
-          # Also, problematic: <http://bugs.icu-project.org/trac/ticket/11333>
-          'U_HAVE_STD_STRING=0',
+          'U_HAVE_STD_STRING=1',
           # TODO(srl295): reenable following pending
           # https://code.google.com/p/v8/issues/detail?id=3345
           # (saves some space)

On some systems, there's also 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. edit split this off to #19656

@srl295 srl295 added the i18n-api Issues and PRs related to the i18n implementation. label Mar 5, 2018
@TimothyGu
Copy link
Member

Can you submit a pull request that fixes this?

@srl295 srl295 closed this as completed in 4de7821 Mar 28, 2018
targos pushed a commit that referenced this issue Apr 2, 2018
- node and v8 did not call into std::string previously,
so that access was shut off.
- this fixes compilation for ICU 58.2 (backlevel) but may
be expressed in other versions also.

Fixes: #19151

PR-URL: #19624
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
Reviewed-By: Tiancheng "Timothy" Gu <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
i18n-api Issues and PRs related to the i18n implementation.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants