Summary: | [JSC] Implement IntlNumberFormat v3 | ||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Yusuke Suzuki <ysuzuki> | ||||||||||||||||||||||||||||||||||||||||||
Component: | JavaScriptCore | Assignee: | 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
Yusuke Suzuki
2020-08-12 20:56:50 PDT
Hmm, many APIs are requiring ICU 69. 1. New useGrouping 2. trailingZeroDisplay 3. halfCeil / halfFloor 4. signDisplay: "negative" Created attachment 436468 [details]
Patch
Created attachment 442293 [details]
Patch
Created attachment 442294 [details]
Patch
Created attachment 442295 [details]
Patch
Created attachment 442296 [details]
Patch
Created attachment 442297 [details]
Patch
Created attachment 442298 [details]
Patch
Created attachment 442299 [details]
Patch
Created attachment 442300 [details]
Patch
Created attachment 442302 [details]
Patch
Created attachment 442304 [details]
Patch
Created attachment 442305 [details]
Patch
Created attachment 442464 [details]
Patch
Created attachment 442472 [details]
Patch
Created attachment 442476 [details]
Patch
Created attachment 442502 [details]
Patch
Created attachment 442511 [details]
Patch
Created attachment 442540 [details]
Patch
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. Created attachment 442549 [details]
Patch
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? (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. Created attachment 442558 [details]
Patch
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 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. Committed r285418 (243975@main): <https://commits.webkit.org/243975@main> Committed r285420 (243976@main): <https://commits.webkit.org/243976@main> Committed r285506 (244030@main): <https://commits.webkit.org/244030@main> |