Skip to content
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

(overflowing|checked)_(add|sub)_offset implementations #1069

Merged
merged 2 commits into from
Sep 25, 2023

Conversation

pitdicker
Copy link
Collaborator

@pitdicker pitdicker commented May 18, 2023

Split out from #1048.

The main goal is to add NaiveDateTime::checked_(add|sub)_offset methods. To quote from #1048:

Instead of panicking in the Add or Sub implementation of NaiveDateTime with FixedOffset, we need a way to be informed of out-of-range result.

I added the following methods (not public for now):

  • NaiveTime::overflowing_add_offset and NaiveTime::overflowing_sub_offset
  • NaiveDateTime::checked_add_offset and NaiveDateTime::checked_sub_offset

The Add and Sub implementations of FixedOffset with NaiveTime, NaiveDateTime and DateTime are reimplemented using of these methods. This fixes a code comment:

this should be implemented more efficiently, but for the time being, this is generic right now.

The best place to put the Add and Sub implementations for DateTime is in the module of DateTime, because there they have access to its private fields. I have moved all implementations to the module of their output type for consistency.


Adding an offset works differently from adding a duration. Adding a duration tries to do something smart—but still rudimentary—with leap seconds. Adding an offset should not touch the leap seconds at all. So the methods that operate on NaiveTime should be different.

I extracted the part that operates on the NaiveDate that could be shared into an add_days methods. Previously NaiveDate::checked_add_days would convert the days to a Duration in seconds, and later devide them to get the value in days again. This now works in a less roundabout way. Might This would also help with the const implementation later.

@pitdicker pitdicker force-pushed the fixed_offset_add_impls branch 4 times, most recently from 4ac7509 to 114fec8 Compare May 19, 2023 14:12
Copy link
Contributor

@jtmoon79 jtmoon79 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approve with docstring nitpicks

src/naive/datetime/mod.rs Outdated Show resolved Hide resolved
src/naive/datetime/mod.rs Outdated Show resolved Hide resolved
@pitdicker
Copy link
Collaborator Author

It seems like I have to rebase stuff after every merged PR to fix merge conflicts. A few more weeks like this and no one can beat me at rebasing 😆.

@pitdicker
Copy link
Collaborator Author

@djc I believe you were halfway done with reviewing this PR in #1071 (comment)? Three other PRs depend on this one.

Not meant to hurry you. Just wanting to highlight this is a good first one of my open PRs.

@djc
Copy link
Member

djc commented Jun 1, 2023

Yup, this is on my list (perhaps counterintuitively, the lack of a waiting-on-review label means I expect to review this sooner rather than the ones labeled as such -- if you have a suggestion for a better label name, I'm all ears).

@pitdicker
Copy link
Collaborator Author

@djc this PR is now quite small and the new NaiveDateTime::checked_add_offset and NaiveDateTime::checked_sub_offset make it easier to write code that avoids panics in other parts of chrono.

@codecov
Copy link

codecov bot commented Sep 2, 2023

Codecov Report

Merging #1069 (018be06) into 0.4.x (c836a1f) will increase coverage by 0.03%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##            0.4.x    #1069      +/-   ##
==========================================
+ Coverage   91.21%   91.25%   +0.03%     
==========================================
  Files          38       38              
  Lines       17121    17114       -7     
==========================================
  Hits        15617    15617              
+ Misses       1504     1497       -7     
Files Changed Coverage Δ
src/datetime/mod.rs 85.22% <0.00%> (-1.03%) ⬇️
src/offset/fixed.rs 82.35% <ø> (+9.21%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Copy link
Member

@djc djc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Old review comment that's been stuck in pending mode...

/// added to a date as a result of the offset (either `-1`, `0`, or `1` because the offset is
/// always less than 24h).
///
/// This method is similar to [`overflowing_add_signed`], but preserves leap seconds.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we replace the implementation of overflowing_add_signed() to use this instead to be more correct?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

overflowing_add_signed() is not incorrect, or at least it works with leap seconds as documented.
Adding one hour to 13:59:60 with overflowing_add_signed should result in 14:59:59 (3600 seconds later).

But when adding a FixedOffset to convert a value from one time zone to another, it should do nothing special with leap seconds.
Adding one hour to 13:59:60 with overflowing_add_offset should result in 14:59:60 (the same value, but shifted by 3600 seconds).

@pitdicker pitdicker force-pushed the fixed_offset_add_impls branch 2 times, most recently from 0910523 to e74dcba Compare September 8, 2023 08:44
@pitdicker
Copy link
Collaborator Author

@djc This is a nice PR for us to push over the finish line.
#1071 to fix a panic in TimeZone::from_local_datetime depends on it, #1048 to fix other panics, #1270 for const initialization macros, and also a PR I want to open to make methods on DateTime const.

@djc
Copy link
Member

djc commented Sep 13, 2023

I'm aware, but you keep submitting other stuff...

@pitdicker
Copy link
Collaborator Author

Haha, yes, that is my fault. But if we want to take some time with little new work to get the existing PRs ready that is fine with me.

@pitdicker
Copy link
Collaborator Author

Added a small optimization to NaiveDateTime::checked_(add|sub)_offset: use succ_opt or pred_opt instead of add_days.
This method is used a lot in the DateTime::with_* methods to convert the value to local time and back to UTC.

Before the add_days optimization #1214:

bench_datetime_with     time:   [52.090 ns 52.417 ns 52.773 ns]
Found 7 outliers among 100 measurements (7.00%)
  6 (6.00%) high mild
  1 (1.00%) high severe

After the add_days optimization:

bench_datetime_with     time:   [46.121 ns 46.263 ns 46.423 ns]
                        change: [-11.281% -10.739% -10.207%] (p = 0.00 < 0.05)
                        Performance has improved.
Found 8 outliers among 100 measurements (8.00%)
  4 (4.00%) high mild
  4 (4.00%) high severe

Now:

bench_datetime_with     time:   [34.281 ns 34.365 ns 34.458 ns]
                        change: [-26.354% -25.942% -25.467%] (p = 0.00 < 0.05)
                        Performance has improved.
Found 10 outliers among 100 measurements (10.00%)
  5 (5.00%) high mild
  5 (5.00%) high severe

In total we are ca. 35% faster in a good number of methods on DateTime.

@djc
Copy link
Member

djc commented Sep 23, 2023

Let's split this in 3 parts?

@pitdicker
Copy link
Collaborator Author

This PR now contains only a tiny part 4.

@@ -1247,6 +1247,16 @@ impl<Tz: TimeZone> AddAssign<Duration> for DateTime<Tz> {
}
}

impl<Tz: TimeZone> Add<FixedOffset> for DateTime<Tz> {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In my opinion this impl doesn't make much sense. Should the offset be added to the datetime, or to the offset? And why? I would like to remove it on main.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removing it on main sounds good to me.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll make a PR soon-ish.

@@ -1247,6 +1247,16 @@ impl<Tz: TimeZone> AddAssign<Duration> for DateTime<Tz> {
}
}

impl<Tz: TimeZone> Add<FixedOffset> for DateTime<Tz> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removing it on main sounds good to me.

@pitdicker pitdicker merged commit d450b68 into chronotope:0.4.x Sep 25, 2023
36 of 37 checks passed
@pitdicker pitdicker deleted the fixed_offset_add_impls branch September 25, 2023 12:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants