Fix TimeZone::get_possible_epoch_ns at date-time limits#580
Merged
nekevss merged 2 commits intoboa-dev:mainfrom Sep 17, 2025
Merged
Fix TimeZone::get_possible_epoch_ns at date-time limits#580nekevss merged 2 commits intoboa-dev:mainfrom
nekevss merged 2 commits intoboa-dev:mainfrom
Conversation
nekevss
requested changes
Sep 16, 2025
Member
nekevss
left a comment
There was a problem hiding this comment.
Thanks for working on this! I have a couple of nits. But then this should be good to go.
869dc22 to
25023bb
Compare
Contributor
Author
|
@nekevss Thanks for the review, done! |
nekevss
approved these changes
Sep 17, 2025
Member
nekevss
left a comment
There was a problem hiding this comment.
Looks good! Thanks for working on this!
Just needs the cfg flag and cargo fmt --all, then this should be good to merge.
This line in the spec text was a mistake. See tc39/proposal-temporal#3151
There is no need to cast to i32 because in most of the places where it is used, it is cast back to i64 anyway. In PlainDate::days_until, where both IsoDate instances can be assumed to be within the valid range, we can cast the difference to i32 instead, since it cannot be larger than 2e8 + 2.
25023bb to
28719fa
Compare
Member
|
Thanks for working on this! |
Contributor
|
Confirmed that this introduces no changes to V8's tests. |
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
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.
The
is_valid_day_range()check was based on an erroneous line in the Temporal spec, see tc39/proposal-temporal#3151. This PR removes that check, and adds aget_possible_epoch_nsunit test for the affected cases.For the unit test it was convenient to have CandidateEpochNanoseconds and EpochNanosecondsAndOffset implement PartialEq. Please let me know if that's not desired and I'll do it another way.
The second commit is a cleanup, not necessary for the bug fix. Feel free to drop it if you prefer not to have it. While I was fixing the above, I noticed that in most places the
to_epoch_daysvalue is cast to i32 and then immediately back to i64, so I changed the return type to i64 and instead moved the cast to the place where i32 is actually needed.