Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[JSC] Support all rounding modes in Temporal #4907

Merged
merged 1 commit into from Oct 2, 2022
Merged

[JSC] Support all rounding modes in Temporal #4907

merged 1 commit into from Oct 2, 2022

Conversation

rkirsling
Copy link
Member

@rkirsling rkirsling commented Oct 1, 2022 •
edited by webkit-early-warning-system

a8f81aa

[JSC] Support all rounding modes in Temporal
https://bugs.webkit.org/show_bug.cgi?id=245935

Reviewed by Geoffrey Garen.

This patch implements the rest of the rounding modes for Temporal, namely,
`expand`, `halfCeil`, `halfFloor`, `halfTrunc`, and `halfEven`.

(As a bonus, it also addresses two adjacent spec tweaks related to fractionalSecondDigits and BalanceDuration.)

Note: Some tests still fail due to precision loss beyond MAX_SAFE_INTEGER...

* JSTests/test262/config.yaml:
* JSTests/test262/expectations.yaml:
* Source/JavaScriptCore/runtime/TemporalDuration.cpp:
(JSC::TemporalDuration::balance):
(JSC::TemporalDuration::total const):
* Source/JavaScriptCore/runtime/TemporalDuration.h:
* Source/JavaScriptCore/runtime/TemporalObject.cpp:
(JSC::temporalFractionalSecondDigits):
(JSC::temporalRoundingMode):
(JSC::negateTemporalRoundingMode):
(JSC::roundNumberToIncrement):

Canonical link: https://commits.webkit.org/255068@main

37bf979

Misc iOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ❌ 🛠 wpe ❌ 🛠 🧪 win
✅ 🛠 ios-sim ✅ 🛠 mac-debug ✅ 🛠 gtk ✅ 🛠 wincairo
✅ 🧪 webkitperl   🧪 ios-wk2   🛠 mac-AS-debug ✅ 🧪 gtk-wk2
✅ 🧪 api-ios   🧪 api-mac ✅ 🧪 api-gtk
✅ 🛠 🧪 jsc ✅ 🛠 tv ✅ 🧪 mac-wk1 ✅ 🛠 jsc-armv7
✅ 🛠 tv-sim ✅ 🧪 mac-wk2 ✅ 🧪 jsc-armv7-tests
✅ 🛠 🧪 merge ✅ 🛠 watch   🧪 mac-AS-debug-wk2 ✅ 🛠 jsc-mips
✅ 🛠 watch-sim ✅ 🧪 mac-wk2-stress ✅ 🧪 jsc-mips-tests

@rkirsling rkirsling requested a review from a team as a code owner October 1, 2022 21:43
@rkirsling rkirsling self-assigned this Oct 1, 2022
@rkirsling rkirsling added JavaScriptCore For bugs in JavaScriptCore, the JS engine used by WebKit, other than kxmlcore issues. WebKit Nightly Build labels Oct 1, 2022
@webkit-early-warning-system
Copy link
Collaborator

EWS run on current version of this PR (hash 37bf979)

Misc iOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ❌ 🛠 wpe ❌ 🛠 🧪 win
✅ 🛠 ios-sim ✅ 🛠 mac-debug ✅ 🛠 gtk ✅ 🛠 wincairo
✅ 🧪 webkitperl   🧪 ios-wk2   🛠 mac-AS-debug ✅ 🧪 gtk-wk2
✅ 🧪 api-ios   🧪 api-mac ✅ 🧪 api-gtk
✅ 🛠 🧪 jsc ✅ 🛠 tv ✅ 🧪 mac-wk1 ✅ 🛠 jsc-armv7
✅ 🛠 tv-sim ✅ 🧪 mac-wk2 ✅ 🧪 jsc-armv7-tests
✅ 🛠 🧪 merge ✅ 🛠 watch   🧪 mac-AS-debug-wk2 ✅ 🛠 jsc-mips
✅ 🛠 watch-sim ✅ 🧪 mac-wk2-stress ✅ 🧪 jsc-mips-tests

@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Oct 1, 2022
@rkirsling rkirsling removed the merging-blocked Applied to prevent a change from being merged label Oct 1, 2022
@rkirsling
Copy link
Member Author

(WPE EWS seems to be unstable at the moment.)

geoffreygaren
geoffreygaren reviewed Oct 1, 2022
View reviewed changes
Source/JavaScriptCore/runtime/TemporalDuration.cpp
@@ -549,7 +553,9 @@ double TemporalDuration::total(JSGlobalObject* globalObject, JSValue optionsValu
}

ISO8601::Duration newDuration = m_duration;
balance(newDuration, unit);
auto infiniteResult = balance(newDuration, unit);
if (infiniteResult)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I kind of prefer "infiniteResult.has_value()" in code like this. Otherwise it's too easy to read this as a check for zero. I'm not sure this is a project-wide consensus though.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, seems like this could be one of those JSC vs. everybody else things. 😅 .has_value()) (with trailing paren) has nearly no results in JSC, despite having a bunch in WebCore & WebKit. Since we've been using this style quite a bit in neighboring code, I'm inclined to leave it as-is, as long as you don't object.

geoffreygaren
geoffreygaren approved these changes Oct 1, 2022
View reviewed changes
Copy link
Contributor

@geoffreygaren geoffreygaren left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

r=me

@rkirsling
Copy link
Member Author

Thanks for the review!

@rkirsling
Copy link
Member Author

(Win EWS seems to be encountering some unrelated flaky layout tests.)

@rkirsling rkirsling added the merge-queue Applied to send a pull request to merge-queue label Oct 2, 2022
@rkirsling
[JSC] Support all rounding modes in Temporal
a8f81aa 
https://bugs.webkit.org/show_bug.cgi?id=245935

Reviewed by Geoffrey Garen.

This patch implements the rest of the rounding modes for Temporal, namely,
`expand`, `halfCeil`, `halfFloor`, `halfTrunc`, and `halfEven`.

(As a bonus, it also addresses two adjacent spec tweaks related to fractionalSecondDigits and BalanceDuration.)

Note: Some tests still fail due to precision loss beyond MAX_SAFE_INTEGER...

* JSTests/test262/config.yaml:
* JSTests/test262/expectations.yaml:
* Source/JavaScriptCore/runtime/TemporalDuration.cpp:
(JSC::TemporalDuration::balance):
(JSC::TemporalDuration::total const):
* Source/JavaScriptCore/runtime/TemporalDuration.h:
* Source/JavaScriptCore/runtime/TemporalObject.cpp:
(JSC::temporalFractionalSecondDigits):
(JSC::temporalRoundingMode):
(JSC::negateTemporalRoundingMode):
(JSC::roundNumberToIncrement):

Canonical link: https://commits.webkit.org/255068@main
@webkit-early-warning-system webkit-early-warning-system force-pushed the eng/JSC-Support-all-rounding-modes-in-Temporal branch from 37bf979 to a8f81aa Compare October 2, 2022 00:54
@webkit-commit-queue
Copy link
Collaborator

Committed 255068@main (a8f81aa): https://commits.webkit.org/255068@main

Reviewed commits have been landed. Closing PR #4907 and removing active labels.

@webkit-early-warning-system webkit-early-warning-system merged commit a8f81aa into WebKit:main Oct 2, 2022
@webkit-commit-queue webkit-commit-queue removed the merge-queue Applied to send a pull request to merge-queue label Oct 2, 2022
@rkirsling rkirsling deleted the eng/JSC-Support-all-rounding-modes-in-Temporal branch October 2, 2022 02:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@geoffreygaren geoffreygaren geoffreygaren approved these changes

Assignees

@rkirsling rkirsling

Labels
JavaScriptCore For bugs in JavaScriptCore, the JS engine used by WebKit, other than kxmlcore issues.
Projects
None yet
Milestone
No milestone
5 participants
@rkirsling @webkit-early-warning-system @webkit-commit-queue @geoffreygaren @webkit-ews-buildbot