Replace icu::UnicodeString with ICU C API and std::u16string

googlefonts / sfntly

A Library for Using, Editing, and Creating SFNT-based Fonts

451 stars 162 forks source link

Replace icu::UnicodeString with ICU C API and std::u16string #105

Closed gvictor closed 5 years ago

gvictor commented 5 years ago

When sfntly depends on ICU4C a shared library, the shared library may not provide stable C++ ABI. Use ICU C API instead.

gvictor commented 5 years ago

FYI. I can't compile this with ICU 57, but successfully compiled it (except gtest) with ICU 63.

It's like due to the change in UChar in ICU 59

Starting with ICU 59, C++ API uses char16_t directly, while C API continues to use UChar.

leizleiz commented 5 years ago

With this change, sample/chromium/subsetter_impl.cc no longer builds in Chromium. See https://chromium-review.googlesource.com/c/chromium/src/+/1398519

gvictor commented 5 years ago

Taking a look. Here is the error. ../../third_party/sfntly/src/cpp/src/sample/chromium/subsetter_impl.cc:55:13: error: no viable overloaded '='

gvictor commented 5 years ago

leizleiz@, what is the ICU version in Chromium? I wonder if it's related to the UChar change in ICU 59

leizleiz commented 5 years ago

https://chromium.googlesource.com/chromium/deps/icu.git/+log/d6530149/README.chromium says 63.1.

I also cannot build sfntly standalone with libicu-dev 57.1-9. How did you successfully built sfntly with this?

gvictor commented 5 years ago

I successfully built in Android with ICU 63. http://r.android.com/855009

I think I have tried to the standalone sfntly with the following command with the Android's ICU 63 headers and modified the suffix to "#define U_ICU_VERSION_SUFFIX _57". "make chrome_subsetter sfntly"

Seems Chromium defines UCHAR_TYPE

if (is_win) {
  defines += [ "UCHAR_TYPE=wchar_t" ]
} else {
  defines += [ "UCHAR_TYPE=uint16_t" ]
}

https://chromium.googlesource.com/chromium/deps/icu.git/+/master/BUILD.gn#59 https://chromium.googlesource.com/chromium/deps/icu.git/+/master/source/common/unicode/umachine.h#337

Let me see if I can make the code more portable for Chromium,

gvictor commented 5 years ago

@leizleiz, could you this patch in Chromium? https://github.com/googlei18n/sfntly/pull/107

leizleiz commented 5 years ago

It works locally. Will try on all platforms shortly.

leizleiz commented 5 years ago

I kicked off a build: https://chromium-review.googlesource.com/c/chromium/src/+/1398519/4, with https://chromium-review.googlesource.com/c/chromium/src/+/1398519/1 being the prior patch set with the failure.