Bug 215438

Summary: [JSC] Implement IntlNumberFormat v3
Product: WebKit Reporter: Yusuke Suzuki <ysuzuki>
Component: JavaScriptCoreAssignee: Yusuke Suzuki <ysuzuki>
Status: RESOLVED FIXED    
Severity: Normal CC: ews-watchlist, keith_miller, mark.lam, msaboff, ross.kirsling, saam, tzagallo, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=232889
https://bugs.webkit.org/show_bug.cgi?id=232949
Bug Depends on:    
Bug Blocks: 218829    
Attachments:
Description Flags
Patch
none
Patch
ews-feeder: commit-queue-
Patch
ews-feeder: commit-queue-
Patch
none
Patch
ews-feeder: commit-queue-
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch ross.kirsling: review+, ews-feeder: commit-queue-

Description Yusuke Suzuki 2020-08-12 20:56:50 PDT
...
Comment 1 Yusuke Suzuki 2020-08-12 20:57:56 PDT
https://unicode-org.atlassian.net/browse/ICU-21182
Comment 2 Radar WebKit Bug Importer 2020-08-19 20:57:13 PDT
<rdar://problem/67449278>
Comment 3 Yusuke Suzuki 2021-08-24 12:33:44 PDT
Hmm, many APIs are requiring ICU 69.

1. New useGrouping
2. trailingZeroDisplay
3. halfCeil / halfFloor
4. signDisplay: "negative"
Comment 4 Yusuke Suzuki 2021-08-25 21:57:30 PDT
Created attachment 436468 [details]
Patch
Comment 5 Yusuke Suzuki 2021-10-24 00:55:54 PDT
Created attachment 442293 [details]
Patch
Comment 6 Yusuke Suzuki 2021-10-24 01:10:08 PDT
Created attachment 442294 [details]
Patch
Comment 7 Yusuke Suzuki 2021-10-24 01:13:33 PDT
Created attachment 442295 [details]
Patch
Comment 8 Yusuke Suzuki 2021-10-24 02:07:14 PDT
Created attachment 442296 [details]
Patch
Comment 9 Yusuke Suzuki 2021-10-24 02:13:07 PDT
Created attachment 442297 [details]
Patch
Comment 10 Yusuke Suzuki 2021-10-24 02:17:14 PDT
Created attachment 442298 [details]
Patch
Comment 11 Yusuke Suzuki 2021-10-24 02:56:23 PDT
Created attachment 442299 [details]
Patch
Comment 12 Yusuke Suzuki 2021-10-24 03:33:36 PDT
Created attachment 442300 [details]
Patch
Comment 13 Yusuke Suzuki 2021-10-24 04:14:00 PDT
Created attachment 442302 [details]
Patch
Comment 14 Yusuke Suzuki 2021-10-24 04:21:41 PDT
Created attachment 442304 [details]
Patch
Comment 15 Yusuke Suzuki 2021-10-24 04:25:46 PDT
Created attachment 442305 [details]
Patch
Comment 16 Yusuke Suzuki 2021-10-25 22:19:30 PDT
Created attachment 442464 [details]
Patch
Comment 17 Yusuke Suzuki 2021-10-26 01:25:23 PDT
Created attachment 442472 [details]
Patch
Comment 18 Yusuke Suzuki 2021-10-26 03:29:17 PDT
Created attachment 442476 [details]
Patch
Comment 19 Yusuke Suzuki 2021-10-26 09:56:15 PDT
Created attachment 442502 [details]
Patch
Comment 20 Yusuke Suzuki 2021-10-26 11:18:01 PDT
Created attachment 442511 [details]
Patch
Comment 21 Yusuke Suzuki 2021-10-26 15:47:50 PDT
Created attachment 442540 [details]
Patch
Comment 22 Yusuke Suzuki 2021-10-26 15:53:08 PDT
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.
Comment 23 Yusuke Suzuki 2021-10-26 17:52:09 PDT
Created attachment 442549 [details]
Patch
Comment 24 Ross Kirsling 2021-10-26 18:36:56 PDT
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?
Comment 25 Yusuke Suzuki 2021-10-26 19:37:36 PDT
(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.
Comment 26 Yusuke Suzuki 2021-10-26 19:46:19 PDT
Created attachment 442558 [details]
Patch
Comment 27 Ross Kirsling 2021-10-26 19:59:48 PDT
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`?
Comment 28 Yusuke Suzuki 2021-10-26 20:17:12 PDT
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.
Comment 29 Yusuke Suzuki 2021-11-08 11:20:54 PST
Committed r285418 (243975@main): <https://commits.webkit.org/243975@main>
Comment 30 Yusuke Suzuki 2021-11-08 11:34:35 PST
Committed r285420 (243976@main): <https://commits.webkit.org/243976@main>
Comment 31 Yusuke Suzuki 2021-11-09 10:11:47 PST
Committed r285506 (244030@main): <https://commits.webkit.org/244030@main>