Bug 230033 – [JSC] Implement Temporal.PlainDate

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

[Yusuke Suzuki]

...

[Radar WebKit Bug Importer]

<rdar://problem/83127747>

[Yusuke Suzuki]

Created attachment 440747 [details]
Patch

WIP

[Dean Jackson]

Created attachment 451827 [details]
Patch

[Dean Jackson]

It seems the non-Apple bots are not generating the lut.h file, but I don't understand why not. DerivedSources.make has been updated.

[Dean Jackson]

Created attachment 451837 [details]
Patch

[Dean Jackson]

This is not intended to implement all of the API, but provide enough to easily add the rest. Yusuke's original patch is a more complete implementation - so I intend to merge bits from that as I go.

[Dean Jackson]

Created attachment 452041 [details]
Patch

[Yusuke Suzuki]

Comment on attachment 452041 [details]
Patch

r=me

[EWS]

Tools/Scripts/svn-apply failed to apply attachment 452041 [details] to trunk.
Please resolve the conflicts and upload a new patch.

[Dean Jackson]

Committed r290209 (247537@trunk): <https://commits.webkit.org/247537@trunk>

[Yusuke Suzuki]

I'll extract my old patch's parsing code and upload it separately.

[Philip Chimento]

Comment on attachment 452041 [details]
Patch

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

Thanks! Although I'm not a WebKit reviewer I've taken a quick look at this from the Temporal perspective and noted some things that could be covered in one of your follow-ups :-)

What would be a good way to keep track of what parts of PlainDate are done and what still needs doing?

> Source/JavaScriptCore/runtime/ISO8601.cpp:992
> +    return makeString(pad('0', 4, plainDate.year()), '-', pad('0', 2, plainDate.month()), '-', pad('0', 2, plainDate.day()));

Just a note for future improvements: this should be able to output 6-digit extended years if the year is outside the range 1000-9999.

> Source/JavaScriptCore/runtime/TemporalPlainDate.cpp:87
> +static ISO8601::PlainDate toPlainDate(JSGlobalObject* globalObject, ISO8601::Duration&& duration)

Nothing wrong with this usage of ISO8601::Duration as a "bag of ISO units" but maybe we should rename it to something like ISO8601::Units, or create ISO8601::Units as a separate type without the other Duration-like methods from Duration?

> Source/JavaScriptCore/runtime/TemporalPlainDate.cpp:120
> +

There's a step missing here (a call to ISODateTimeWithinLimits, https://tc39.es/proposal-temporal/#_ref_222) although I don't know if it belongs in this method or in toPlainDate() or create().

[Yusuke Suzuki]

Comment on attachment 452041 [details]
Patch

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

>> Source/JavaScriptCore/runtime/TemporalPlainDate.cpp:120
>> +
> 
> There's a step missing here (a call to ISODateTimeWithinLimits, https://tc39.es/proposal-temporal/#_ref_222) although I don't know if it belongs in this method or in toPlainDate() or create().

This is done in https://bugs.webkit.org/show_bug.cgi?id=236936