Various tweaks in preparation for Temporal.Instant
Created attachment 438310 [details] Patch
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.
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)`
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.
Created attachment 438421 [details] Patch
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 :-)
Any idea why the `"T".repeat(int32_max)` test might be crashing on armv7?
(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?
<rdar://problem/83425035>
Created attachment 438998 [details] Patch
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?
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.
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.
Created attachment 439052 [details] Patch
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].