Bug 230331 – Various tweaks in preparation for Temporal.Instant

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Philip Chimento]

Various tweaks in preparation for Temporal.Instant

[Philip Chimento]

Created attachment 438310 [details]
Patch

[Yusuke Suzuki]

Comment on attachment 438310 [details]
Patch

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

> Source/JavaScriptCore/runtime/TemporalDuration.cpp:146
> -            throwRangeError(globalObject, scope, "Could not parse Duration string"_s);
> +            throwRangeError(globalObject, scope, makeString("'"_s, string, "' is not a valid Duration string"_s));

Need to handle the case string is INT32_MAX size already.
We need to truncate the string if it is too long.

> Source/JavaScriptCore/runtime/TemporalObject.cpp:65
> -static JSValue createNowObject(VM& vm, JSObject* object)
> +static JSValue createDurationConstructor(VM& vm, JSObject* object)
>  {
>      TemporalObject* temporalObject = jsCast<TemporalObject*>(object);
>      JSGlobalObject* globalObject = temporalObject->globalObject(vm);
> -    return TemporalNow::create(vm, TemporalNow::createStructure(vm, globalObject));
> +    return TemporalDurationConstructor::create(vm, TemporalDurationConstructor::createStructure(vm, globalObject, globalObject->functionPrototype()), jsCast<TemporalDurationPrototype*>(globalObject->durationStructure()->storedPrototypeObject()));
>  }
>  
> -static JSValue createDurationConstructor(VM& vm, JSObject* object)
> +static JSValue createNowObject(VM& vm, JSObject* object)
>  {
>      TemporalObject* temporalObject = jsCast<TemporalObject*>(object);
>      JSGlobalObject* globalObject = temporalObject->globalObject(vm);
> -    return TemporalDurationConstructor::create(vm, TemporalDurationConstructor::createStructure(vm, globalObject, globalObject->functionPrototype()), jsCast<TemporalDurationPrototype*>(globalObject->durationStructure()->storedPrototypeObject()));
> +    return TemporalNow::create(vm, TemporalNow::createStructure(vm, globalObject));
>  }
>  

I don't think these changes are necessary.

> Source/JavaScriptCore/runtime/TemporalObject.cpp:171
> +String temporalUnitToString(TemporalUnit unit)
> +{
> +    switch (unit) {
> +    case TemporalUnit::Year: return "year"_s;
> +    case TemporalUnit::Month: return "month"_s;
> +    case TemporalUnit::Week: return "week"_s;
> +    case TemporalUnit::Day: return "day"_s;
> +    case TemporalUnit::Hour: return "hour"_s;
> +    case TemporalUnit::Minute: return "minute"_s;
> +    case TemporalUnit::Second: return "second"_s;
> +    case TemporalUnit::Millisecond: return "millisecond"_s;
> +    case TemporalUnit::Microsecond: return "microsecond"_s;
> +    case TemporalUnit::Nanosecond: return "nanosecond"_s;
> +    }
> +    ASSERT_NOT_REACHED();
> +}

Use TemporalDuration.cpp's propertyName.

[Yusuke Suzuki]

Comment on attachment 438310 [details]
Patch

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

>> Source/JavaScriptCore/runtime/TemporalDuration.cpp:146
>> +            throwRangeError(globalObject, scope, makeString("'"_s, string, "' is not a valid Duration string"_s));
> 
> Need to handle the case string is INT32_MAX size already.
> We need to truncate the string if it is too long.

Can you add a test passing super long string? `"T".repeat(int32_max)`

[Ross Kirsling]

Comment on attachment 438310 [details]
Patch

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

> Source/JavaScriptCore/runtime/ISO8601.h:65
> +    double& operator[](TemporalUnit u) { return m_data[static_cast<uint8_t>(u)]; }
> +    const double& operator[](TemporalUnit u) const { return m_data[static_cast<uint8_t>(u)]; }

Hmm, it doesn't seem like you're using this in this patch, though -- I think it should be added just when the need arises.

[Philip Chimento]

Created attachment 438421 [details]
Patch

[Philip Chimento]

Thanks for the reviews. I've removed the functions that are not directly used in this patch. That combined with rebasing this over recent changes has made this patch much smaller :-)

[Philip Chimento]

Any idea why the `"T".repeat(int32_max)` test might be crashing on armv7?

[Philip Chimento]

(In reply to Philip Chimento from comment #7)
> Any idea why the `"T".repeat(int32_max)` test might be crashing on armv7?

I am reasonably sure now this is because the EWS bots just run out of memory when you do this. The tests seem to crash with SIGKILL not SIGSEGV. I've been trying this on a rpi4 and I also get SIGKILL when just running `'T'.repeat(2147483647)` at the JSC shell prompt. So maybe we shouldn't have this test after all?

[Radar WebKit Bug Importer]

<rdar://problem/83425035>

[Philip Chimento]

Created attachment 438998 [details]
Patch

[Philip Chimento]

For now I've removed the test that exhausts the armv7 buildbot's memory. Also new in this revision is a small bugfix to handle { fractionalSecondDigits: NaN } according to the specification. Finally I moved TemporalDuration::unitPropertyName() to TemporalObject.cpp as discussed with Ross, and renamed it temporalUnitPropertyName().

If we want the T.repeat(int32max) test back, maybe there is a way to disable it on the buildbots that can't handle it?

[Ross Kirsling]

Comment on attachment 438998 [details]
Patch

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

> Source/JavaScriptCore/runtime/TemporalObject.cpp:242
> +        if (std::isnan(doubleValue) || doubleValue < 0 || doubleValue > 9) {

Ahh, we could do it this way, but this is actually me being silly -- I should have written `if(!(doubleValue >= 0 && doubleValue <= 9))`, as Yusuke did in TemporalPlainTime.

[Yusuke Suzuki]

Comment on attachment 438998 [details]
Patch

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

r=me with comments

> Source/JavaScriptCore/runtime/TemporalDuration.cpp:122
> +        copy.append(UChar { 0x2026 }); // U+2026 ellipsis

Use `horizontalEllipsis` in <wtf/unicode/CharacterNames.h>

> Source/JavaScriptCore/runtime/TemporalObject.cpp:254
> +        throwRangeError(globalObject, scope, makeString("fractionalSecondDigits must be 'auto' or 0 through 9, not "_s, stringValue));

Also need to consider about super long string. So ellipsizeAt is necessary.

[Philip Chimento]

Created attachment 439052 [details]
Patch

[EWS]

Committed r283009 (242074@main): <https://commits.webkit.org/242074@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 439052 [details].