Temporal: Make ExactTime::difference() return an InternalDuration#45603
Conversation
|
EWS run on previous version of this PR (hash f90e056) Details |
f90e056 to
abff2e7
Compare
|
EWS run on previous version of this PR (hash abff2e7) Details |
| return round(diff, increment, unit, roundingMode); | ||
| } | ||
| VM& vm = globalObject->vm(); | ||
| auto scope = DECLARE_THROW_SCOPE(vm); |
There was a problem hiding this comment.
I think it's necessary, because ExactTime::difference() calls roundTimeDuration(), which can throw.
There was a problem hiding this comment.
Never mind, I misunderstood. Fixed in b6b5887
| return ExactTime { round(m_epochNanoseconds, increment, unit, roundingMode) }; | ||
| Int128 timeDuration = other.m_epochNanoseconds - m_epochNanoseconds; | ||
| timeDuration = roundTimeDuration(globalObject, timeDuration, roundingIncrement, smallestUnit, roundingMode); | ||
| RETURN_IF_EXCEPTION(scope, { }); |
There was a problem hiding this comment.
I think it is? roundTimeDuration() can throw.
| TemporalUnit unit, RoundingMode roundingMode) | ||
| { | ||
| VM& vm = globalObject->vm(); | ||
| auto scope = DECLARE_THROW_SCOPE(vm); |
| auto scope = DECLARE_THROW_SCOPE(vm); | ||
|
|
||
| auto divisor = lengthInNanoseconds(unit); | ||
| RELEASE_AND_RETURN(scope, roundTimeDurationToIncrement(globalObject, timeDuration, |
There was a problem hiding this comment.
Just use return roundTimeDurationToIncrement(...).
There was a problem hiding this comment.
I thought it had to be RELEASE_AND_RETURN(...) because roundTimeDurationToIncrement() can throw?
There was a problem hiding this comment.
I'm not sure if this is what @Constellation is referring to, but maybe the tail-call style would just use roundTimeDurationToIncrement's throw scope and creating an outer one wouldn't be necessary, since this is the only thing in the scope that can throw? Just a guess, I don't know off the top of my head if that's accurate.
ptomato
left a comment
There was a problem hiding this comment.
I'm not a reviewer, but I had some suggestions that will hopefully help the actual review.
|
|
||
| // since() and until() | ||
| { | ||
| const earlier = Temporal.Instant.from("1976-11-18T15:23:30.123456789Z"); | ||
| const later = Temporal.Instant.from("2019-10-29T10:46:38.271986102Z"); | ||
| const diff = later.since(earlier); | ||
| shouldBe(diff.nanoseconds, 313); | ||
| shouldBe(diff.microseconds, 529); | ||
| shouldBe(diff.milliseconds, 148); | ||
| shouldBe(diff.seconds, 1355167388); | ||
| const diff1 = earlier.until(later); | ||
| shouldBe(diff1.nanoseconds, diff.nanoseconds); | ||
| shouldBe(diff1.microseconds, diff.microseconds); | ||
| shouldBe(diff1.milliseconds, diff.milliseconds); | ||
| shouldBe(diff1.seconds, diff.seconds); | ||
| } |
There was a problem hiding this comment.
Optional: I think this is probably sufficiently covered by test262 and doesn't need an additional stress test; can't hurt though
There was a problem hiding this comment.
I thought that all changes were supposed to have stress tests, but I agree it does overlap with test262.
| microseconds = microseconds % 1000; | ||
| } else if (largestUnit == TemporalUnit::Microsecond) { | ||
| microseconds = (Int128) std::trunc(std::fma<double, unsigned, double>( | ||
| seconds, 6, std::trunc(((double) nanoseconds) / 1000))); |
There was a problem hiding this comment.
I think 6 is probably meant to be 10⁶? If so, this should make more test262 tests pass (and if it doesn't, we should add coverage for this)
There was a problem hiding this comment.
Good catch! It turns out the code was correct because seconds is always 0. (I got there from following the code in the polyfill, but in the polyfill time durations are represented with seconds and subseconds separately, and in JSC it's a single nanoseconds value.) But the call to std::fma is unnecessary so I changed it in c719f90 to be similar to the other cases.
| if (increment == 1) | ||
| return x; | ||
|
|
||
| // The following code follows the polyfill rather than the spec, because we don't have float128. |
There was a problem hiding this comment.
I would suggest rephrasing this comment and the one below not to refer to "the polyfill", it's not clear from context here what is meant.
There was a problem hiding this comment.
I added links to the relevant code in the polyfill in 86a28f8 -- hopefully that clarifies?
|
EWS run on previous version of this PR (hash c719f90) Details |
|
EWS run on previous version of this PR (hash 86a28f8) Details |
|
EWS run on previous version of this PR (hash 1a7478f) Details |
|
EWS run on previous version of this PR (hash b6b5887) Details |
|
EWS run on previous version of this PR (hash 8bfea14) Details |
| case TemporalUnit::Hour: maximum = (Int128) WTF::hoursPerDay; break; | ||
| case TemporalUnit::Minute: maximum = (Int128) (minutesPerHour * WTF::hoursPerDay); break; | ||
| case TemporalUnit::Second: maximum = (Int128) (secondsPerMinute * minutesPerHour * WTF::hoursPerDay); break; | ||
| case TemporalUnit::Millisecond: maximum = (Int128) msPerDay; break; | ||
| case TemporalUnit::Microsecond: maximum = (Int128) msPerDay * 1000; break; |
There was a problem hiding this comment.
You shouldn't need the (Int128) and if for some reason it is needed WebKit style would be to static_cast<Int128> rather than C-style cast.
There was a problem hiding this comment.
I think the cast was necessary to get it to compile on armv7, but I'll try it without.
There was a problem hiding this comment.
Yup, armv7 fails if I remove the casts; I re-added them in the static_cast form.
|
EWS run on previous version of this PR (hash 9ba1c59) Details |
|
EWS run on previous version of this PR (hash 5d20d61) Details |
|
The win-tests failure seems pretty squarely unrelated. Tim's out today, but I'd like to move this forward as much as possible. I'll see if I can dig up the next patch in his series and submit that. |
|
@ptomato does not have committer permissions according to https://raw.githubusercontent.com/WebKit/WebKit/main/metadata/contributors.json. If you do have committer permissions, please ensure that your GitHub username is added to contributors.json. Rejecting 5d20d61df4e4a34ef12020a32641d4ce7927a3f5 from merge queue. |
|
(Sorry, wrong label.) |
|
This change contains multiple commits which are not squashed together, blocking PR #45603. Please squash the commits to land. |
5d20d61 to
0622f71
Compare
|
EWS run on current version of this PR (hash 0622f71) Details |
|
Rebased and squashed. |
https://bugs.webkit.org/show_bug.cgi?id=223166 Reviewed by Keith Miller. TemporalInstant::difference() now calls ExactTime::difference(), then converts the result back to a Duration. This enables some of the tests for TemporalInstant/p/since and TemporalInstant/p/until that previously failed. As part of this change, implement ExactTime::round() according to the spec. * JSTests/stress/temporal-instant.js: * JSTests/test262/expectations.yaml: * Source/JavaScriptCore/runtime/ISO8601.cpp: (JSC::ISO8601::roundTemporalInstant): (JSC::ISO8601::validateTemporalRoundingIncrement): (JSC::ISO8601::ExactTime::round const): (JSC::ISO8601::roundTimeDurationToIncrement): (JSC::ISO8601::roundTimeDuration): (JSC::ISO8601::ExactTime::difference const): (JSC::ISO8601::absInt128): Deleted. (JSC::ISO8601::ExactTime::round): Deleted. * Source/JavaScriptCore/runtime/ISO8601.h: (JSC::ISO8601::ExactTime::absInt128): * Source/JavaScriptCore/runtime/TemporalDuration.cpp: (JSC::TemporalDuration::temporalDurationFromInternal): * Source/JavaScriptCore/runtime/TemporalDuration.h: * Source/JavaScriptCore/runtime/TemporalInstant.cpp: * Source/JavaScriptCore/runtime/TemporalObject.cpp: (JSC::applyUnsignedRoundingMode): (JSC::roundNumberToIncrementAsIfPositive): (JSC::roundNumberToIncrementInt128): * Source/JavaScriptCore/runtime/TemporalObject.h: (JSC::lengthInNanoseconds): (JSC::getUnsignedRoundingMode): Canonical link: https://commits.webkit.org/299050@main
0622f71 to
4927ce2
Compare
|
Committed 299050@main (4927ce2): https://commits.webkit.org/299050@main Reviewed commits have been landed. Closing PR #45603 and removing active labels. |
4927ce2
0622f71
🧪 win-tests🧪 api-wpe🛠 playstation