Bug 224093 – ICU 69 Deprecates ubrk_safeClone in favor of ubrk_clone

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Don Olmstead]

...

[Don Olmstead]

Created attachment 424978 [details]
WIP Patch

[Yusuke Suzuki]

Comment on attachment 424978 [details]
WIP Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=424978&action=review

> Source/JavaScriptCore/runtime/IntlSegmenter.cpp:129
> +    auto segmenter = std::unique_ptr<UBreakIterator, UBreakIteratorDeleter>(ubrk_clone(m_segmenter.get(), &status));

Since ubrk_clone is draft status, we need to hide U_HIDE_DRAFT_API definition to use it. So I suggest,

1. Let's create a new cpp/h files, IntlWorkaround.h IntlWorkaround.cpp
2. Annotate @no-unify to IntlWorkaround.cpp since we want to avoid including unicode/ubrk.h without removing U_HIDE_DRAFT_API
3. Define a wrapper function around ubrk_clone / ubrk_safeClone in IntlWorkaround.cpp. To use draft ICU APIs, we need some special handling. Please check IntlListFormat.cpp for example :)

[Yusuke Suzuki]

Comment on attachment 424978 [details]
WIP Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=424978&action=review

>> Source/JavaScriptCore/runtime/IntlSegmenter.cpp:129
>> +    auto segmenter = std::unique_ptr<UBreakIterator, UBreakIteratorDeleter>(ubrk_clone(m_segmenter.get(), &status));
> 
> Since ubrk_clone is draft status, we need to hide U_HIDE_DRAFT_API definition to use it. So I suggest,
> 
> 1. Let's create a new cpp/h files, IntlWorkaround.h IntlWorkaround.cpp
> 2. Annotate @no-unify to IntlWorkaround.cpp since we want to avoid including unicode/ubrk.h without removing U_HIDE_DRAFT_API
> 3. Define a wrapper function around ubrk_clone / ubrk_safeClone in IntlWorkaround.cpp. To use draft ICU APIs, we need some special handling. Please check IntlListFormat.cpp for example :)

Or let's define a helper function in IntlSegmenter.cpp, make it @no-unify, and include the ubrk.h with appropriate or #define.
And ensure that ubrk.h is not included without draft's #define for that file.

[Radar WebKit Bug Importer]

<rdar://problem/76434312>

[Ross Kirsling]

Created attachment 425816 [details]
Patch

[Ross Kirsling]

Created attachment 425817 [details]
Patch

[Ross Kirsling]

Created attachment 425820 [details]
Patch

[Yusuke Suzuki]

Comment on attachment 425820 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=425820&action=review

r=me with some comments.

> Source/JavaScriptCore/runtime/IntlWorkaround.cpp:41
> +#if HAVE(ICU_UBRK_CLONE)
> +#define U_HIDE_DRAFT_API 1
> +#endif

I think we do not need to make it back since this cpp file is isolated.

> Source/JavaScriptCore/runtime/IntlWorkaround.cpp:50
> +return ubrk_clone(iterator, status);

4 spaces for indentation.

> Source/JavaScriptCore/runtime/IntlWorkaround.cpp:52
> +return ubrk_safeClone(iterator, nullptr, nullptr, status);

Ditto.

[Ross Kirsling]

Created attachment 425822 [details]
Patch for landing

[EWS]

Committed r275856 (236421@main): <https://commits.webkit.org/236421@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 425822 [details].

[Darin Adler]

Comment on attachment 425822 [details]
Patch for landing

View in context: https://bugs.webkit.org/attachment.cgi?id=425822&action=review

> Source/JavaScriptCore/runtime/IntlSegmenter.h:80
> +// Abstraction to call ubrk_safeClone or ubrk_clone depending on ICU version.
> +// This is implemented in IntlWorkaround.cpp in order to confine draft API visibility.
> +UBreakIterator* cloneUBreakIterator(const UBreakIterator*, UErrorCode*);

Why is this here instead of an IntlWorkaround.h header?

> Source/JavaScriptCore/runtime/IntlWorkaround.cpp:42
> +UBreakIterator* cloneUBreakIterator(const UBreakIterator*, UErrorCode*);

Why is this declaration repeated here instead of including the header it’s in?

[Ross Kirsling]

(In reply to Darin Adler from comment #11)
> Comment on attachment 425822 [details]
> Patch for landing
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=425822&action=review
> 
> > Source/JavaScriptCore/runtime/IntlSegmenter.h:80
> > +// Abstraction to call ubrk_safeClone or ubrk_clone depending on ICU version.
> > +// This is implemented in IntlWorkaround.cpp in order to confine draft API visibility.
> > +UBreakIterator* cloneUBreakIterator(const UBreakIterator*, UErrorCode*);
> 
> Why is this here instead of an IntlWorkaround.h header?
> 
> > Source/JavaScriptCore/runtime/IntlWorkaround.cpp:42
> > +UBreakIterator* cloneUBreakIterator(const UBreakIterator*, UErrorCode*);
> 
> Why is this declaration repeated here instead of including the header it’s
> in?

Er right, I guess we should be able to put the declaration in an IntlWorkaround.h after all.

(This was meant to be less confusing than introducing a IntlWorkaround.h and having IntlWorkaround.cpp *not* include it. However, the reason for that was because IntlSegmenter.h still needs to include ubrk.h in the usual way and I'd been trying to have IntlWorkaround "manage inclusion" of ubrk.h. But as long as we don't muck with the include in IntlSegmenter.h, then IntlWorkaround.h can safely house a declaration and nothing more.)