-
Notifications
You must be signed in to change notification settings - Fork 515
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
Make remaining methods const #1337
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #1337 +/- ##
=======================================
Coverage 91.84% 91.84%
=======================================
Files 38 38
Lines 17491 17519 +28
=======================================
+ Hits 16064 16090 +26
- Misses 1427 1429 +2 ☔ View full report in Codecov by Sentry. |
src/duration.rs
Outdated
pub fn checked_add(&self, rhs: &Duration) -> Option<Duration> { | ||
let mut secs = try_opt!(self.secs.checked_add(rhs.secs)); | ||
pub const fn checked_add(&self, rhs: &Duration) -> Option<Duration> { | ||
// No overflow checks here because we stay comfortably wihtin the range of an `i64`. |
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: typo wihtin
Same typo on other lines
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.
Thank you, fixed.
// check if `self` is a leap second and adding `rhs` would escape that leap second. | ||
// if it's the case, update `self` and `rhs` to involve no leap second; | ||
// otherwise the addition immediately finishes. | ||
pub const fn overflowing_add_signed(&self, rhs: OldDuration) -> (NaiveTime, i64) { |
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.
There appear to be significant logic changes to overflowing_add_signed
. Currently, test_time_overflowing_add
does not have any asserts involving fractional seconds.
https://github.com/chronotope/chrono/blob/v0.4.31/src/naive/time/tests.rs#L117
I strongly recommend adding more asserts to test_time_overflowing_add
specifically targeting this change in logic.
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.
There appear to be significant logic changes to
overflowing_add_signed
.
True, this method was pretty convoluted because it comes from a time before rem_euclid
, like we had in more places in chrono.
This code is extremely well-tested, better than any other part of chrono. The tests just work on the higher-level methods that wrap test_time_overflowing_add
.
c00d57a
to
3dac37a
Compare
I hoped to make this the last PR before suggesting a 0.4.32 release. And then 4 months evaporated 🙄. |
3dac37a
to
730cbed
Compare
Updated. Now that our MSRV is 1.61 it is possible to also make |
730cbed
to
217afac
Compare
I'll wait with merging until after #1385, to make this the one that has to rebase. |
217afac
to
1668a86
Compare
1668a86
to
ab98148
Compare
ab98148
to
addcf66
Compare
@pitdicker I'm concerned about some of the changes brought in here. I like the new I can understand why this is, but I feel it is a wrong path. Over in #1392 we have discussed reworking some of the constants - it seems to me that this would be a good opportunity to correct some of these patterns at the same time. What do you think? 🙂 Would you be open to me submitting a PR that not only streamlines the constants but also ensures they are used in the various checks that are currently diverse? |
You are right. It would especially be nice to replace the For now I'm glad to have the changes to make the |
pub(crate) const fn new(secs: i64, nanos: u32) -> Option<Duration> { | ||
if secs < MIN.secs | ||
|| secs > MAX.secs | ||
|| nanos > 1_000_000_000 |
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 functions documentation says >= 1_000_000_000
, but here > 1_000_000_000
is used in the check?
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.
Thank you for catching that!
@danwilliams Keep an eye on #1351 that is also touching constants in the |
Depends on #1137.
This finishes the work to make all methods const where possible.
Where it is not possible is parsing and formatting, and anything that involves traits including
DateTime<tz>
.