Bug 215438 – [JSC] Implement IntlNumberFormat v3

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

[Yusuke Suzuki]

...

[Yusuke Suzuki]

https://unicode-org.atlassian.net/browse/ICU-21182

[Radar WebKit Bug Importer]

<rdar://problem/67449278>

[Yusuke Suzuki]

Hmm, many APIs are requiring ICU 69.

1. New useGrouping
2. trailingZeroDisplay
3. halfCeil / halfFloor
4. signDisplay: "negative"

[Yusuke Suzuki]

Created attachment 436468 [details]
Patch

[Yusuke Suzuki]

Created attachment 442293 [details]
Patch

[Yusuke Suzuki]

Created attachment 442294 [details]
Patch

[Yusuke Suzuki]

Created attachment 442295 [details]
Patch

[Yusuke Suzuki]

Created attachment 442296 [details]
Patch

[Yusuke Suzuki]

Created attachment 442297 [details]
Patch

[Yusuke Suzuki]

Created attachment 442298 [details]
Patch

[Yusuke Suzuki]

Created attachment 442299 [details]
Patch

[Yusuke Suzuki]

Created attachment 442300 [details]
Patch

[Yusuke Suzuki]

Created attachment 442302 [details]
Patch

[Yusuke Suzuki]

Created attachment 442304 [details]
Patch

[Yusuke Suzuki]

Created attachment 442305 [details]
Patch

[Yusuke Suzuki]

Created attachment 442464 [details]
Patch

[Yusuke Suzuki]

Created attachment 442472 [details]
Patch

[Yusuke Suzuki]

Created attachment 442476 [details]
Patch

[Yusuke Suzuki]

Created attachment 442502 [details]
Patch

[Yusuke Suzuki]

Created attachment 442511 [details]
Patch

[Yusuke Suzuki]

Created attachment 442540 [details]
Patch

[Yusuke Suzuki]

Comment on attachment 442540 [details]
Patch

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

> Source/JavaScriptCore/ChangeLog:18
> +            - halfCeil / halfFloor (requires ICU 68)
> +            - signDisplay: "negative" (requires ICU 68)

Ah, they are ICU 69.

> Source/JavaScriptCore/ChangeLog:19
> +            - formatRangeToParts (requires ICU 69)

And ICU 70.

[Yusuke Suzuki]

Created attachment 442549 [details]
Patch

[Ross Kirsling]

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

Fantastic. :D

> Source/JavaScriptCore/runtime/IntlNumberFormat.cpp:402
> +    bool invalidRoundingIncrement = true;
> +    for (unsigned candidate : roundingIncrementCandidates) {
> +        if (m_roundingIncrement == candidate) {
> +            invalidRoundingIncrement = false;
> +            break;
> +        }
> +    }
> +    if (invalidRoundingIncrement) {

How about std::none_of? :)

> Source/JavaScriptCore/runtime/IntlNumberFormat.cpp:839
> +    if (std::signbit(end) && !end && start >= 0)

I wonder if we should have an inlined helper function for negative zero?

> Source/JavaScriptCore/runtime/IntlNumberFormat.h:212
> +    enum class TrailingZeroDisplay : uint8_t { Auto, StripIfInteger };

Would there be any benefit to having this (and CompactDisplay) be bool?

> Source/JavaScriptCore/runtime/IntlNumberFormat.h:214
> +    enum class RoundingMode : uint8_t { Ceil, Floor, Expand, Trunc, HalfCeil, HalfFloor, HalfExpand, HalfTrunc, HalfEven };

Sounds like Temporal is intended to use the same RoundingMode values as NumberFormat V3, so I think we should move and update the existing enum.
(See editor's note @ https://tc39.es/proposal-temporal/#sec-temporal-totemporalroundingmode)

> Source/JavaScriptCore/runtime/IntlNumberFormatInlines.h:71
> +    bool hasSd = !minimumSignificantDigitsValue.isUndefined() || !maximumSignificantDigitsValue.isUndefined();
> +    bool hasFd = !minimumFractionDigitsValue.isUndefined() || !maximumFractionDigitsValue.isUndefined();
> +    bool needSd = hasSd || roundingPriority != IntlRoundingPriority::Auto;
> +    bool needFd = (!hasSd && notation != IntlNotation::Compact) || roundingPriority != IntlRoundingPriority::Auto;

nit: Seems like it should be "has"/"needs" or "have"/"need". Also if we're going to abbreviate, I think it should be "SD" and "FD".

> JSTests/ChangeLog:10
> +        * stress/intl-numberformat-format-range-v3.js: Added.

Seems like many of the new test files just deal with en-US -- maybe we should ensure that each new feature is getting checked for multiple locales?

> JSTests/stress/intl-numberformat-format-range-v3.js:54
> +    ['formatRange'].forEach(function(method) {

Why the forEach?

[Yusuke Suzuki]

(In reply to Ross Kirsling from comment #24)
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=442540&action=review
> 
> Fantastic. :D
> 
> > Source/JavaScriptCore/runtime/IntlNumberFormat.cpp:402
> > +    bool invalidRoundingIncrement = true;
> > +    for (unsigned candidate : roundingIncrementCandidates) {
> > +        if (m_roundingIncrement == candidate) {
> > +            invalidRoundingIncrement = false;
> > +            break;
> > +        }
> > +    }
> > +    if (invalidRoundingIncrement) {
> 
> How about std::none_of? :)

Sounds good.

> 
> > Source/JavaScriptCore/runtime/IntlNumberFormat.cpp:839
> > +    if (std::signbit(end) && !end && start >= 0)
> 
> I wonder if we should have an inlined helper function for negative zero?

Added isNegativeZero.

> 
> > Source/JavaScriptCore/runtime/IntlNumberFormat.h:212
> > +    enum class TrailingZeroDisplay : uint8_t { Auto, StripIfInteger };
> 
> Would there be any benefit to having this (and CompactDisplay) be bool?

Useful for stringifying function (strongly typed enum ensures the right parameter is passed).
And this is aligned to the other options.
Since it is not true/false option, we should use uint8_t.


> 
> > Source/JavaScriptCore/runtime/IntlNumberFormat.h:214
> > +    enum class RoundingMode : uint8_t { Ceil, Floor, Expand, Trunc, HalfCeil, HalfFloor, HalfExpand, HalfTrunc, HalfEven };
> 
> Sounds like Temporal is intended to use the same RoundingMode values as
> NumberFormat V3, so I think we should move and update the existing enum.
> (See editor's note @
> https://tc39.es/proposal-temporal/#sec-temporal-totemporalroundingmode)

Changed.

> 
> > Source/JavaScriptCore/runtime/IntlNumberFormatInlines.h:71
> > +    bool hasSd = !minimumSignificantDigitsValue.isUndefined() || !maximumSignificantDigitsValue.isUndefined();
> > +    bool hasFd = !minimumFractionDigitsValue.isUndefined() || !maximumFractionDigitsValue.isUndefined();
> > +    bool needSd = hasSd || roundingPriority != IntlRoundingPriority::Auto;
> > +    bool needFd = (!hasSd && notation != IntlNotation::Compact) || roundingPriority != IntlRoundingPriority::Auto;
> 
> nit: Seems like it should be "has"/"needs" or "have"/"need". Also if we're
> going to abbreviate, I think it should be "SD" and "FD".

This is aligned to the spec's names strictly since this is complicated.

> 
> > JSTests/ChangeLog:10
> > +        * stress/intl-numberformat-format-range-v3.js: Added.
> 
> Seems like many of the new test files just deal with en-US -- maybe we
> should ensure that each new feature is getting checked for multiple locales?

Added some tests.

> 
> > JSTests/stress/intl-numberformat-format-range-v3.js:54
> > +    ['formatRange'].forEach(function(method) {
> 
> Why the forEach?

Because originally it also included "formatRangeToParts", which cannot be implemented with the current ICU version.
I left forEach since I would like to keep V8 test as is as much as possible.

[Yusuke Suzuki]

Created attachment 442558 [details]
Patch

[Ross Kirsling]

Comment on attachment 442558 [details]
Patch

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

r=me

> Source/JavaScriptCore/runtime/IntlNumberFormat.cpp:422
> +    m_roundingMode = intlOption<RoundingMode>(globalObject, options, vm.propertyNames->roundingMode, {

I think ultimately we should update temporalRoundingMode and use it here (or actually probably turn it into intlRoundingMode and use it from Temporal)?
But I guess we could wait for that spec to actually get updated with the intended behaviors.

> Source/JavaScriptCore/runtime/MathCommon.h:68
> +    return std::signbit(value) && value == 0;

I think the style checker prefers `!value`?

[Yusuke Suzuki]

Comment on attachment 442558 [details]
Patch

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

>> Source/JavaScriptCore/runtime/IntlNumberFormat.cpp:422
>> +    m_roundingMode = intlOption<RoundingMode>(globalObject, options, vm.propertyNames->roundingMode, {
> 
> I think ultimately we should update temporalRoundingMode and use it here (or actually probably turn it into intlRoundingMode and use it from Temporal)?
> But I guess we could wait for that spec to actually get updated with the intended behaviors.

Yes. Since TemporalObject and the spec is not handling these options, we should keep it separated for now.

>> Source/JavaScriptCore/runtime/MathCommon.h:68
>> +    return std::signbit(value) && value == 0;
> 
> I think the style checker prefers `!value`?

I think it does not work if value is signed NaN.

[Yusuke Suzuki]

Committed r285418 (243975@main): <https://commits.webkit.org/243975@main>

[Yusuke Suzuki]

Committed r285420 (243976@main): <https://commits.webkit.org/243976@main>

[Yusuke Suzuki]

Committed r285506 (244030@main): <https://commits.webkit.org/244030@main>