Fix panic in DateTime::checked_add_days#941
Conversation
Incidentally, add TimeDelta::try_* builders so that it's possible to build them without risking a panic.
0f1a635 to
9262c43
Compare
This is a backport of chronotope#941, except it needs to work around the fact that we can't modify the `time` crate.
This is a backport of chronotope#941, except it needs to work around the fact that we can't modify the `time` crate.
This is a backport of chronotope#941, except it needs to work around the fact that we can't modify the `time` crate.
|
Going to close this. We'll review (and probably merge) #942 (the 0.4.x version of this change) and then eventually that change will make it to the main branch as part of a regular merge of 0.4.x work into main. |
|
Hmm so I had opened both because this fix is IMO better than the one in #941 , as it can actually add some API surface to |
|
Ahh, sorry, had missed that context. Will open this back up, then we can review it after the other change has made its way into main. |
|
Awesome thanks! :) |
This is a backport of #941, except it needs to work around the fact that we can't modify the `time` crate.
|
(Merge is pending in #986.) |
|
Can you rebase this onto main? Should hopefully not be too painful. |
esheppa
left a comment
There was a problem hiding this comment.
Looks good to me, pending rebase
jtmoon79
left a comment
There was a problem hiding this comment.
Can you add test cases for the before and after for all changed functions?
e.g. test_diff_days, test_weeks, test_try_weeks, etc.
Specifically, two commits
- a commit with new test cases
- a commit of these changes + adjustments to those test cases (if needed)
|
Sorry for the delay. I have basically not done any non-work software development for a few months, and have a few personal projects that will be higher priority for me than this PR, so I don't think I'll actually come back to this anytime soon. Do you prefer if I leave the PR open so that someone else can easily notice it and take it over (and so that it tracks the fact that the panic is currently reachable on |
|
@Ekleog That is very reasonable. If it is ok with you I can take over in a couple of days. |
|
That'd be perfect, thank you! :) |
|
The new methods are included in #1137 together with other features for |
Incidentally, add TimeDelta::try_* builders so that it's possible to build them without risking a panic.
This issue was found thanks to cargo-bolero-fuzzing my web server, without actually trying to fuzz chrono itself.