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.
nit: typo wihtin
Same typo on other lines
There was a problem hiding this comment.
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.
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.
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.
pitdicker
force-pushed
the
const_remaining
branch
2 times, most recently
from
c00d57a
to
3dac37a
Compare
|
I hoped to make this the last PR before suggesting a 0.4.32 release. And then 4 months evaporated 🙄. |
pitdicker
force-pushed
the
const_remaining
branch
from
3dac37a
to
730cbed
Compare
|
Updated. Now that our MSRV is 1.61 it is possible to also make |
pitdicker
force-pushed
the
const_remaining
branch
from
730cbed
to
217afac
Compare
|
I'll wait with merging until after #1385, to make this the one that has to rebase. |
pitdicker
force-pushed
the
const_remaining
branch
from
217afac
to
1668a86
Compare
pitdicker
force-pushed
the
const_remaining
branch
from
1668a86
to
ab98148
Compare
pitdicker
force-pushed
the
const_remaining
branch
from
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.
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.
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>.