...
Created attachment 424978 [details] WIP Patch
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 :)
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.
<rdar://problem/76434312>
Created attachment 425816 [details] Patch
Created attachment 425817 [details] Patch
Created attachment 425820 [details] Patch
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.
Created attachment 425822 [details] Patch for landing
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].
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?
(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.)