[webkit-reviews] review granted: [Bug 169458] [INTL] Implement Intl.DateTimeFormat.prototype.formatToParts : [Attachment 307547] Patch

[bugzilla-daemon at webkit.org]

JF Bastien <jfbastien at apple.com> has granted Andy VanWagoner
<thetalecrafter at gmail.com>'s request for review:
Bug 169458: [INTL] Implement Intl.DateTimeFormat.prototype.formatToParts
https://bugs.webkit.org/show_bug.cgi?id=169458

Attachment 307547: Patch

https://bugs.webkit.org/attachment.cgi?id=307547&action=review




--- Comment #39 from JF Bastien <jfbastien at apple.com> ---
Comment on attachment 307547
  --> https://bugs.webkit.org/attachment.cgi?id=307547
Patch

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

Two minor things to address, then LGTM.

> Source/JavaScriptCore/runtime/IntlDateTimeFormat.cpp:45
> +#if U_ICU_VERSION_MAJOR_NUM >= 55

Could you move the version 55 check to a header, and conditionally define a
macro such as:

#if U_ICU_VERSION_MAJOR_NUM >= 55
#define JSC_ICU_HAS_UFIELDPOSITER 1
#endif

So after that you just use that macro, and only ever check for version 55 in a
single spot.

> Source/JavaScriptCore/runtime/IntlDateTimeFormat.cpp:957
> +    case UDAT_TIME_SEPARATOR_FIELD:

It looks like UDAT_TIME_SEPARATOR_FIELD is new to ICU 55, so this wouldn't
compile (and partTypeString isn't compiled conditionally)?