[webkit-reviews] review granted: [Bug 228532] [JSC] Implement Temporal.Duration : [Attachment 436911] Patch

[bugzilla-daemon at webkit.org]

Yusuke Suzuki <ysuzuki at apple.com> has granted Ross Kirsling
<ross.kirsling at sony.com>'s request for review:
Bug 228532: [JSC] Implement Temporal.Duration
https://bugs.webkit.org/show_bug.cgi?id=228532

Attachment 436911: Patch

https://bugs.webkit.org/attachment.cgi?id=436911&action=review




--- Comment #24 from Yusuke Suzuki <ysuzuki at apple.com> ---
Comment on attachment 436911
  --> https://bugs.webkit.org/attachment.cgi?id=436911
Patch

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

r=me

> Source/JavaScriptCore/runtime/ISO8601.cpp:43
> +	   auto padded = makeString(fractionString, "00000000");

Maybe we should use Vector<LChar, 9 + 8> buffer to avoid heap allocation here
(We know that all fractionString's characters are ASCIIDigit, so it should be
fit in LChar. And we know the maximum length is 9. And padding length is 8. So
9 + 8 should be enough.
But it is OK for the subsequent patch for optimization.

> Source/JavaScriptCore/runtime/ISO8601.cpp:57
> +	   duration.setMinutes(std::trunc(fraction));

OK, this is a spec bug.

> Source/JavaScriptCore/runtime/ISO8601.cpp:97
> +    if (buffer.lengthRemaining() < 3)
> +	   return std::nullopt;

This looks nice. Let's add a test cases like,

""
"+"
"+P"
"-P"
"+-P"

> Source/JavaScriptCore/runtime/ISO8601.cpp:136
> +	   if (toASCIIUpper(*buffer) == 'Y' && !datePartIndex) {
> +	       result.setYears(integer);
> +	       datePartIndex = 1;
> +	   } else if (toASCIIUpper(*buffer) == 'M' && datePartIndex < 2) {
> +	       result.setMonths(integer);
> +	       datePartIndex = 2;
> +	   } else if (toASCIIUpper(*buffer) == 'W' && datePartIndex < 3) {
> +	       result.setWeeks(integer);
> +	       datePartIndex = 3;
> +	   } else if (toASCIIUpper(*buffer) == 'D') {
> +	       result.setDays(integer);
> +	       datePartIndex = 4;
> +	   } else
> +	       return std::nullopt;

Using switch would be better for readability?

switch (toASCIIUpper(*buffer)) {
case 'Y': {
    if (datePartIndex)
	return std::nullopt;
    result.setYears(integer);
    datePartIndex = 1;
    break;
}
...
default:
    return std::nullopt;
}

> Source/JavaScriptCore/runtime/ISO8601.cpp:163
> +	       if (!digits || digits > 9)
> +		   return std::nullopt;

Looks nice. Let's add a test case like,

"+P20.S"
"+P.20S"

> Source/JavaScriptCore/runtime/ISO8601.cpp:171
> +	   if (toASCIIUpper(*buffer) == 'H' && !timePartIndex) {

Ditto about switch.

> Source/JavaScriptCore/runtime/ISO8601.cpp:175
> +		   timePartIndex = 3;

This trick is nice.

> Source/JavaScriptCore/runtime/ISO8601.h:48
> +    std::array<double, numberOfTemporalUnits> data;

Let's have a default initialization here since std::array can be uninitialized.

std::array<double, numberOfTemporalUnits> data { };

> Source/JavaScriptCore/runtime/TemporalDuration.cpp:202
> +    if (one->years() || two->years() || one->months() || two->months() ||
one->weeks() || two->weeks()) {
> +	   throwRangeError(globalObject, scope, "Cannot compare a duration of
years, months, or weeks without a relativeTo option"_s);
> +	   return { };
> +    }

Let's have a FIXME for handling this case when Temporal DateTime is supported.

> Source/JavaScriptCore/runtime/TemporalDuration.cpp:387
> +	   static const double nanosecondsPerDay = 24.0 * 60 * 60 * 1000 * 1000
* 1000;

It appears twice, so let's define it as a global static constexpr value.

> Source/JavaScriptCore/runtime/TemporalObject.cpp:149
> +    if (largestUnit == "auto")

Use "auto"_s.