[JSC] Implement Intl.DurationFormat #4589
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Constellation
added
JavaScriptCore
|
EWS run on previous version of this PR (hash fab2b91) |
Constellation
force-pushed
the
eng/JSC-Implement-Intl-DurationFormat
branch
from
fab2b91 to
fc3ee47
Compare
|
EWS run on previous version of this PR (hash fc3ee47) |
🛠 ios
Constellation
force-pushed
the
eng/JSC-Implement-Intl-DurationFormat
branch
from
fc3ee47 to
cfea900
Compare
|
EWS run on previous version of this PR (hash cfea900) |
webkit-ews-buildbot
added
the
merging-blocked
Constellation
removed
the
merging-blocked
Constellation
force-pushed
the
eng/JSC-Implement-Intl-DurationFormat
branch
from
cfea900 to
8515478
Compare
|
EWS run on previous version of this PR (hash 8515478) |
webkit-ews-buildbot
added
the
merging-blocked
Constellation
removed
the
merging-blocked
Constellation
force-pushed
the
eng/JSC-Implement-Intl-DurationFormat
branch
from
8515478 to
5407f68
Compare
|
EWS run on previous version of this PR (hash 5407f68) |
Constellation
force-pushed
the
eng/JSC-Implement-Intl-DurationFormat
branch
from
5407f68 to
3c368e2
Compare
|
EWS run on previous version of this PR (hash 3c368e2) |
Constellation
force-pushed
the
eng/JSC-Implement-Intl-DurationFormat
branch
from
3c368e2 to
d2703af
Compare
|
EWS run on previous version of this PR (hash d2703af) |
Constellation
force-pushed
the
eng/JSC-Implement-Intl-DurationFormat
branch
from
d2703af to
4b4a4e0
Compare
|
EWS run on previous version of this PR (hash 4b4a4e0) |
Constellation
force-pushed
the
eng/JSC-Implement-Intl-DurationFormat
branch
from
4b4a4e0 to
6f72813
Compare
|
EWS run on previous version of this PR (hash 6f72813) |
Constellation
force-pushed
the
eng/JSC-Implement-Intl-DurationFormat
branch
from
6f72813 to
9b7d100
Compare
|
EWS run on previous version of this PR (hash 9b7d100) |
rkirsling
approved these changes
Sep 23, 2022
Comment on lines
+74
to
+89
| class UnitData { | ||
| public: | ||
| UnitData() = default; | ||
| UnitData(UnitStyle style, Display display) | ||
| : m_style(style) | ||
| , m_display(display) | ||
| { | ||
| } | ||
|
|
||
| UnitStyle style() const { return m_style; } | ||
| Display display() const { return m_display; } | ||
|
|
||
| private: | ||
| UnitStyle m_style : 7 { UnitStyle::Long }; | ||
| Display m_display : 1 { Display::Always }; | ||
| }; |
There was a problem hiding this comment.
Why not just a struct?
Suggested change
| class UnitData { | |
| public: | |
| UnitData() = default; | |
| UnitData(UnitStyle style, Display display) | |
| : m_style(style) | |
| , m_display(display) | |
| { | |
| } | |
| UnitStyle style() const { return m_style; } | |
| Display display() const { return m_display; } | |
| private: | |
| UnitStyle m_style : 7 { UnitStyle::Long }; | |
| Display m_display : 1 { Display::Always }; | |
| }; | |
| struct UnitData { | |
| UnitStyle style : 7 { UnitStyle::Long }; | |
| Display display : 1 { Display::Always }; | |
| }; |
There was a problem hiding this comment.
That's fine. Changed.
There was a problem hiding this comment.
Ah, it causes some compile error.
/Users/yusukesuzuki/dev/OpenSource/Source/JavaScriptCore/runtime/IntlDurationFormat.cpp:237:25: error: non-const reference cannot bind to bit-field 'm_style'
prevStyle = unitData.m_style;
I'll just use the current class.
Comment on lines
+114
to
+122
| // 1. Let availableLocales be %DurationFormat%.[[availableLocales]]. | ||
| const auto& availableLocales = intlDurationFormatAvailableLocales(); | ||
|
|
||
| // 2. Let requestedLocales be CanonicalizeLocaleList(locales). | ||
| Vector<String> requestedLocales = canonicalizeLocaleList(globalObject, callFrame->argument(0)); | ||
| RETURN_IF_EXCEPTION(scope, encodedJSValue()); | ||
|
|
||
| // 3. Return SupportedLocales(availableLocales, requestedLocales, options). | ||
| RELEASE_AND_RETURN(scope, JSValue::encode(supportedLocales(globalObject, availableLocales, requestedLocales, callFrame->argument(1)))); |
There was a problem hiding this comment.
Hmm, I'm not really sure the step-by-step comments are helping in this case.
There was a problem hiding this comment.
It is currently aligned to the rest of Intl code.
Comment on lines
112
to
113
| IntlDurationFormat::UnitStyle style = styleValue.value_or(IntlDurationFormat::UnitStyle::Long); | ||
| if (!styleValue) { |
There was a problem hiding this comment.
Seems weird to use value_or if we're guaranteed not to use the default value.
Suggested change
| IntlDurationFormat::UnitStyle style = styleValue.value_or(IntlDurationFormat::UnitStyle::Long); | |
| if (!styleValue) { | |
| IntlDurationFormat::UnitStyle style; | |
| if (styleValue) | |
| style = styleValue.value(); | |
| else { |
Comment on lines
254
to
+256
| #if HAVE(ICU_U_LIST_FORMATTER) | ||
| if (Options::useIntlDurationFormat()) | ||
| putDirectWithoutTransition(vm, vm.propertyNames->DurationFormat, createDurationFormatConstructor(vm, this), static_cast<unsigned>(PropertyAttribute::DontEnum)); |
There was a problem hiding this comment.
Seems inconsistent that we're using #if HAVE(ICU_U_LIST_FORMATTER) to guard both specific places in the implementation and Intl.DurationFormat's whole existence.
I guess ListFormat is similar, but I think it's weird to design errors that can never be thrown.
There was a problem hiding this comment.
It is just for reducing compile time errors. We do not want to scatter this ifdefs outside of IntlDurationFormat.cpp as much as possible. So we confine them in that cpp file and this place by doing this.
There was a problem hiding this comment.
I guess what I was thinking is that you could #if the whole IntlDurationFormat implementation instead of specific parts?
There was a problem hiding this comment.
This makes LUT table generation etc. hard to maintain on non-build platforms (and we need to put ifdefs in IntlDurationPrototype.cpp etc. too), so, this is avoiding that.
| } | ||
|
|
||
| #if HAVE(ICU_U_LIST_FORMATTER) | ||
| class ListFormatInput { |
There was a problem hiding this comment.
Why not share this with IntlListFormat? It seems the same.
Constellation
force-pushed
the
eng/JSC-Implement-Intl-DurationFormat
branch
from
9b7d100 to
e71a4f8
Compare
|
EWS run on current version of this PR (hash e71a4f8) |
|
EWS run on current version of this PR (hash e71a4f8) |
Constellation
added
525.x (Safari 3.1)
merge-queue
webkit-ews-buildbot
added
merging-blocked
Constellation
added
unsafe-merge-queue
|
The failure is unrelated flaky test |
https://bugs.webkit.org/show_bug.cgi?id=214794 <rdar://66436701> Reviewed by Ross Kirsling. This patch implements Intl.DurationFormat[1], which is now stage-3 feature. It is decoupled from Temporal now: it can take an object with `hours` etc. properties, and generate formatted string for duration. In the future, it will accept Temporal.Duration too. 1. We add new microsecond and nanosecond units because they are necessary to DurationFormat[2]. 2. Implement Intl.DurationFormat with UNumberFormatter and UListFormatter. [1]: https://github.com/tc39/proposal-intl-duration-format [2]: tc39/ecma402#708 * JSTests/stress/intl-durationformat-basic.js: Added. (shouldBe): (shouldBeOneOf): (shouldBeForICUVersion): (shouldNotThrow): (shouldThrow): * JSTests/stress/intl-durationformat-digital.js: Added. (shouldBe): (throw.new.Error): * JSTests/stress/intl-durationformat-format-to-parts.js: Added. (shouldBe): (shouldBeOneOf): (shouldBeForICUVersion): (shouldNotThrow): (shouldThrow): (throw.new.Error): * JSTests/stress/intl-durationformat.js: Added. (shouldBe): (shouldNotThrow): (shouldThrow): (test.DerivedDurationFormat): (test.get shouldThrow): (test): * JSTests/stress/intl-enumeration.js: * JSTests/test262/config.yaml: * JSTests/test262/expectations.yaml: * Source/JavaScriptCore/CMakeLists.txt: * Source/JavaScriptCore/DerivedSources-input.xcfilelist: * Source/JavaScriptCore/DerivedSources-output.xcfilelist: * Source/JavaScriptCore/DerivedSources.make: * Source/JavaScriptCore/JavaScriptCore.xcodeproj/project.pbxproj: * Source/JavaScriptCore/Sources.txt: * Source/JavaScriptCore/heap/Heap.cpp: (JSC::Heap::Heap): * Source/JavaScriptCore/heap/Heap.h: * Source/JavaScriptCore/heap/HeapSubspaceTypes.h: * Source/JavaScriptCore/runtime/CommonIdentifiers.h: * Source/JavaScriptCore/runtime/IntlDurationFormat.cpp: Added. (JSC::IntlDurationFormat::create): (JSC::IntlDurationFormat::createStructure): (JSC::IntlDurationFormat::IntlDurationFormat): (JSC::IntlDurationFormat::finishCreation): (JSC::intlDurationUnitOptions): (JSC::displayName): (JSC::IntlDurationFormat::initializeDurationFormat): (JSC::ListFormatInput::ListFormatInput): (JSC::ListFormatInput::size const): (JSC::ListFormatInput::stringPointers const): (JSC::ListFormatInput::stringLengths const): (JSC::retrieveSeparator): (JSC::collectElements): (JSC::IntlDurationFormat::format const): (JSC::IntlDurationFormat::formatToParts const): (JSC::IntlDurationFormat::resolvedOptions const): (JSC::IntlDurationFormat::styleString): (JSC::IntlDurationFormat::unitStyleString): (JSC::IntlDurationFormat::displayString): * Source/JavaScriptCore/runtime/IntlDurationFormat.h: Added. * Source/JavaScriptCore/runtime/IntlDurationFormatConstructor.cpp: Added. (JSC::IntlDurationFormatConstructor::create): (JSC::IntlDurationFormatConstructor::createStructure): (JSC::IntlDurationFormatConstructor::IntlDurationFormatConstructor): (JSC::IntlDurationFormatConstructor::finishCreation): (JSC::JSC_DEFINE_HOST_FUNCTION): * Source/JavaScriptCore/runtime/IntlDurationFormatConstructor.h: Added. * Source/JavaScriptCore/runtime/IntlDurationFormatPrototype.cpp: Added. (JSC::IntlDurationFormatPrototype::create): (JSC::IntlDurationFormatPrototype::createStructure): (JSC::IntlDurationFormatPrototype::IntlDurationFormatPrototype): (JSC::IntlDurationFormatPrototype::finishCreation): (JSC::JSC_DEFINE_HOST_FUNCTION): * Source/JavaScriptCore/runtime/IntlDurationFormatPrototype.h: Added. * Source/JavaScriptCore/runtime/IntlObject.cpp: (JSC::createDurationFormatConstructor): (JSC::IntlObject::finishCreation): * Source/JavaScriptCore/runtime/IntlObject.h: (JSC::intlDurationFormatAvailableLocales): * Source/JavaScriptCore/runtime/JSGlobalObject.cpp: (JSC::JSGlobalObject::init): * Source/JavaScriptCore/runtime/JSGlobalObject.h: (JSC::JSGlobalObject::durationFormatStructure): Canonical link: https://commits.webkit.org/254791@main
webkit-early-warning-system
force-pushed
the
eng/JSC-Implement-Intl-DurationFormat
branch
from
e71a4f8 to
0a14082
Compare
|
Committed 254791@main (0a14082): https://commits.webkit.org/254791@main Reviewed commits have been landed. Closing PR #4589 and removing active labels. |
webkit-commit-queue
removed
the
unsafe-merge-queue
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
0a14082
e71a4f8
🛠 mac-debug🧪 mac-wk1🛠 🧪 merge