-
Notifications
You must be signed in to change notification settings - Fork 224
Add exhaustive tests for calendar round tripping and from_fields
and fix overflow bugs
#7065
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.
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
Conversation
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. |
d3314c7
to
852c96d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still have a preference for avoiding release panics. Fix looks good, though.
} | ||
} | ||
|
||
const LONG_YEAR_LEN: u16 = 355; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
question: why remove these?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
uh I tried to refactor some of the simulated code and prefer them inline, because then you see that the three cases it checks are continuous values
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair enough. I find that the constant name also serves as documentation, so perhaps include a short comment in the new hardcoded constants saying what they are? Non blocking.
I know we're not consistent about documenting "obvious" per-calendar internals but it's nice to try.
components/calendar/src/cal/hijri.rs
Outdated
while rd >= year.start_day() + year.last_day_of_month(12) as i64 { | ||
year = self.0.year_data(year.extended_year + 1) | ||
} | ||
year = self.0.year_data(year.extended_year - 1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thought: still a little wary about removing the while loop.
In policy and needed for #7065
from_fields
from_fields
and fix overflow bugs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(github is buggy and is not letting me post these comments without a review message)
.div_euclid(duration.0); | ||
base_moment + duration * num_periods | ||
let diff_millis = (rata_die - base_moment.rata_die) as i128 | ||
* MILLISECONDS_IN_EPHEMERIS_DAY as i128 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this PR is attempting to extend those bounds to ±2³⁰.
With this PR, all calendars work with that wide range, as opposed to most calendars working there except for chinese, dangi, and Hijri (and Hijri is a very simple fix).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(github didn't record my review)
Github review functionality has been really buggy these days, often on the "files changed" view if you hit refresh (with nothing in the URL query or fragment) it still shows only a part of the changes. Beware. |
fff5205
to
7b882d3
Compare
7b882d3
to
b5d2087
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fuzzer changes have been fixed, and Chinese changes have been reverted for now (aside from a non-behavior-changing refactor).
I'm fine with this, and I think none of my open comments are blocking. Shane still has a PartialOrd concern it seems, unsure if that's blocking.
let diff_millis = (rata_die - base_moment.rata_die) * MILLISECONDS_IN_EPHEMERIS_DAY | ||
- base_moment.local_milliseconds as i64; | ||
|
||
let num_periods = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: ideally, explain each step. There's a -1 here that is now easier to document
This is a preexisting issue that need not be fixed here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The PR looks fine overall. I defer to @Manishearth on the review of the test as well as the changes to Hijri. I trust you or @Manishearth to decide that my comments are sufficiently addressed and dismiss my review and merge when ready.
let millis = base_moment.rata_die.to_i64_date() * MILLISECONDS_IN_EPHEMERIS_DAY | ||
+ base_moment.local_milliseconds as i64 | ||
+ num_periods * duration.0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: I don't see how inlining the impl core::ops::Add<Milliseconds> for LocalMoment
and impl core::ops::Mul<i64> for Milliseconds
fixes any overflow bugs. Please un-do the inlining or explain why you need to inline it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it doesn't fix overflow bugs: previously this code did, but then we asked to not upgrade Milliseconds
to i128
, so now this leftover refactor doesn't really do anything. I think it's overall cleaner, at least in this function, but I think it's fine to split that out into a separate PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I removed the Mul impl because it's more debuggable to have an overflow panic here than there
@sffc none of you comments seem blocking, why are you blocking this PR? |
My outstanding issues are about code comments and clarity; please fix them, but can be done in a follow-up if this blocks other words
Co-authored-by: Shane F. Carr <[email protected]>
This tests the
RataDie -> Date<C> -> Date<Iso> -> Date<C> -> RataDie
round trip for all dates between-270000-01-01
and270000-12-31
. It also asserts that the producedDate<C>
follows the same ordering as the associatedRataDie
(some calendars already test this for some ranges). For this purpose I added theDateInner: PartialOrd
bound toCalendar
. Four calendars, (includingAnyCalendar
, which is why it can't beOrd
) were lacking this implementation.This discovered a bug in
Hijri
, where the start of the year was stored relative to the approximatef64
mean synodic year length, not the exact tabular year length (this was missed in #7057). I changed thedebug_assertion!
s in the Hijri code to be actual errors, which are unwrapped in const1 for hardcoded data, and can be shown to not happen at runtime because the only year infos we create are tabular, which obeys all the invariants (the simulated rules now fall back to tabular for years where they would break invariants, I couldn't care less about this calendar). These invariants also let us remove the while loops added in #7057.This PR also adds an exhaustive test for
from_fields
over the extended years-270000..=270000
, all month codes, days in1..=100
and ordinal months in1..=100
.Both tests require
release-with-assertions
to run in an appropriate time, but the round trip tests then finishes in seconds. We should figure out some way to run at least that one in CI, as it would supersede a lot of manually written tests for specific calendars and ranges, and provides valuable coverage.Footnotes
This requires MSRV 1.83, which is more than 6 versions old, so I can use ↩