Update to new ComputeNudgeWindow spec text #636
Draft
+228
−38
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Manishearth
force-pushed
the
nudge-window
branch
from
d3594c8 to
9a445cb
Compare
Contributor
Author
|
This now passes tests, but I'm not entirely sure of the impl |
Manishearth
force-pushed
the
nudge-window
branch
from
9a445cb to
6394a10
Compare
Contributor
Author
|
This fails other tests (due to hitting assertions, I think) |
nekevss
reviewed
Nov 27, 2025
Member
nekevss
left a comment
nekevss
left a comment
There was a problem hiding this comment.
Have a couple notes after taking a glance.
Are you still looking into the failures mentioned above?
| // 7. Let r1 be nudgeWindow.[[R1]]. | ||
| // 8. Let r2 be nudgeWindow.[[R2]]. | ||
| // 9. Set startEpochNs to nudgeWindow.[[StartEpochNs]]. | ||
| // 10. Set endEpochNs to nudgeWindow.[[StartEpochNs]]. |
Contributor
There was a problem hiding this comment.
tc39/proposal-temporal#3191 - should have been caught in review, but anba caught it a bit later
| (dest_epoch_ns - start_epoch_ns.0) as f64 / (end_epoch_ns.0 - start_epoch_ns.0) as f64; | ||
| // 14. Let progress be (destEpochNs - startEpochNs) / (endEpochNs - startEpochNs). | ||
| // 15. Let total be r1 + progress × increment × sign. | ||
| let progress = (dest_epoch_ns.0 - start_epoch_ns.0) as f64 |
Member
There was a problem hiding this comment.
note: this actually may be a candidate for using the Fraction type to compute
|
|
||
| impl UnsignedRoundingMode { | ||
| /// <https://tc39.es/proposal-temporal/#sec-applyunsignedroundingmode> | ||
| pub(crate) fn apply(self, x: f64, r1: i128, r2: i128) -> i128 { |
Member
There was a problem hiding this comment.
issue: I think at the point that we are passing x as a f64, we should just pass r1 and r2 as a f64 as well.
Contributor
Author
There was a problem hiding this comment.
We can't, because r1 and r2 are the actual returned values, and that would potentially change the produced mathematical value.
On the other hand, the only thing that matters about x is which side of r1 + r2 / 2 it is. That's easier to get right.
We should probably use a Fraction type.
Member
There was a problem hiding this comment.
We can't, because r1 and r2 are the actual returned values, and that would potentially change the produced mathematical value.
Ah shoot, true.
On the other hand, the only thing that matters about x is which side of r1 + r2 / 2 it is. That's easier to get right.
Yeah, that's what IncrementRounder was relying on.
| temporal_assert!(r1.abs() as f64 <= total.abs() && total.abs() < r2.abs() as f64); | ||
| // b. Let roundedUnit be ApplyUnsignedRoundingMode(abs(total), abs(r1), abs(r2), unsignedRoundingMode). | ||
| // TODO: what happens to r2 here? | ||
| unsigned_rounding_mode.apply(total.abs(), r1.abs(), r2.abs()) |
Member
There was a problem hiding this comment.
oh, huh. Yeah, it'd probably be worth taking a second pass at rounding given some of the changes to the specification.
I'd really prefer to stay away from f64 if possible, but it might not be with all the changes that have been made to the spec since this was first written.
| } | ||
| } | ||
|
|
||
| impl UnsignedRoundingMode { |
Member
There was a problem hiding this comment.
nit: It would be nice to have some unit tests for this.
Contributor
Author
No, but they should be replicable by copying the tests. |
Manishearth
force-pushed
the
nudge-window
branch
from
6394a10 to
acb80b0
Compare
Contributor
Author
|
The failures were due to a missing <=. Fixed. |
Contributor
Author
|
And that fixes the crashes. Would still like to see if this can be written without floats, but I don't have time to do that myself. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #635
Updates to tc39/proposal-temporal#3172
This passes the test, but it fails other tests. The problem is probably that IncrementRounder isn't written in terms of r1 and r2. @nekevss, I don't understand the full purpose of IncrementRounder being written the way it is: mind having a look?The new spec text changes how r1 and r2 are computed, and they're passed down to IncrementRounder in the spec but not the code, so I assume that IncrementRounder is recomputing them somehow from the inputs.I wrote this to not use IncrementRounder. I'm not 100% sure if the implementation is correct when it comes to how it handles mathematical values.