Bug 230033 - [JSC] Implement Temporal.PlainDate
Summary: [JSC] Implement Temporal.PlainDate
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Yusuke Suzuki
URL:
Keywords: InRadar
Depends on:
Blocks: 223166
  Show dependency treegraph
 
Reported: 2021-09-07 19:29 PDT by Yusuke Suzuki
Modified: 2022-02-21 12:32 PST (History)
13 users (show)

See Also:


Attachments
Patch (95.52 KB, patch)
2021-10-10 17:39 PDT, Yusuke Suzuki
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (63.41 KB, patch)
2022-02-13 11:09 PST, Dean Jackson
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (64.05 KB, patch)
2022-02-13 12:30 PST, Dean Jackson
no flags Details | Formatted Diff | Diff
Patch (64.46 KB, patch)
2022-02-15 09:44 PST, Dean Jackson
ysuzuki: review+
ews-feeder: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yusuke Suzuki 2021-09-07 19:29:11 PDT
...
Comment 1 Radar WebKit Bug Importer 2021-09-14 19:30:16 PDT
<rdar://problem/83127747>
Comment 2 Yusuke Suzuki 2021-10-10 17:39:58 PDT
Created attachment 440747 [details]
Patch

WIP
Comment 3 Dean Jackson 2022-02-13 11:09:12 PST
Created attachment 451827 [details]
Patch
Comment 4 Dean Jackson 2022-02-13 12:28:25 PST
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.
Comment 5 Dean Jackson 2022-02-13 12:30:40 PST
Created attachment 451837 [details]
Patch
Comment 6 Dean Jackson 2022-02-13 13:13:09 PST
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.
Comment 7 Dean Jackson 2022-02-15 09:44:20 PST
Created attachment 452041 [details]
Patch
Comment 8 Yusuke Suzuki 2022-02-18 16:03:34 PST
Comment on attachment 452041 [details]
Patch

r=me
Comment 9 EWS 2022-02-18 20:17:30 PST
Tools/Scripts/svn-apply failed to apply attachment 452041 [details] to trunk.
Please resolve the conflicts and upload a new patch.
Comment 10 Dean Jackson 2022-02-19 09:43:51 PST
Committed r290209 (247537@trunk): <https://commits.webkit.org/247537@trunk>
Comment 11 Yusuke Suzuki 2022-02-19 11:03:07 PST
I'll extract my old patch's parsing code and upload it separately.
Comment 12 Philip Chimento 2022-02-21 12:16:40 PST
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().
Comment 13 Yusuke Suzuki 2022-02-21 12:32:09 PST
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