`(overflowing|checked)_(add|sub)_offset` implementations by pitdicker · Pull Request #1069 · chronotope/chrono

pitdicker · 2023-05-18T05:35:51Z

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.

src/offset/fixed.rs

jtmoon79

Approve with docstring nitpicks

src/naive/datetime/mod.rs

pitdicker · 2023-05-23T11:44:13Z

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 · 2023-06-01T06:34:15Z

@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 · 2023-06-01T07:35:13Z

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 · 2023-08-11T05:56:26Z

@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 · 2023-09-02T06:32:24Z

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

djc

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

djc · 2023-08-07T11:25:32Z

src/naive/time/mod.rs

+    /// 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.


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

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 · 2023-09-12T20:22:22Z

@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 · 2023-09-13T12:40:45Z

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

pitdicker · 2023-09-13T13:33:14Z

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 · 2023-09-23T06:08:36Z

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 · 2023-09-23T07:03:56Z

Let's split this in 3 parts?

pitdicker · 2023-09-25T11:33:48Z

This PR now contains only a tiny part 4.

pitdicker · 2023-09-25T11:51:06Z

src/datetime/mod.rs

    }
 }

+impl<Tz: TimeZone> Add<FixedOffset> for DateTime<Tz> {


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.

Removing it on main sounds good to me.

I'll make a PR soon-ish.

djc · 2023-09-25T12:11:22Z

src/datetime/mod.rs

    }
 }

+impl<Tz: TimeZone> Add<FixedOffset> for DateTime<Tz> {


Removing it on main sounds good to me.

This was referenced May 18, 2023

Fix panic in TimeZone::from_local_datetime #1071

Merged

Fix DateTime out-of-range panics #1048

Closed

djc reviewed May 19, 2023

View reviewed changes

src/offset/fixed.rs Show resolved Hide resolved

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

jtmoon79 approved these changes May 21, 2023

View reviewed changes

src/naive/datetime/mod.rs Outdated Show resolved Hide resolved

src/naive/datetime/mod.rs Outdated Show resolved Hide resolved

pitdicker force-pushed the fixed_offset_add_impls branch from 114fec8 to 1f7645a Compare May 23, 2023 11:40

pitdicker force-pushed the fixed_offset_add_impls branch from 1f7645a to d20c780 Compare May 23, 2023 11:56

pitdicker mentioned this pull request May 23, 2023

Make methods on NaiveTime const, update MSRV to 1.57 #1080

Merged

djc added waiting-on-review and removed waiting-on-review labels May 26, 2023

pitdicker force-pushed the fixed_offset_add_impls branch from d20c780 to f54d7a8 Compare June 8, 2023 17:55

pitdicker force-pushed the fixed_offset_add_impls branch from f54d7a8 to 37e6a98 Compare June 29, 2023 17:21

pitdicker mentioned this pull request Jul 6, 2023

const from_ymd_opt #1172

Merged

pitdicker force-pushed the fixed_offset_add_impls branch from 37e6a98 to ce58270 Compare July 20, 2023 05:09

pitdicker force-pushed the fixed_offset_add_impls branch from ce58270 to 5911afc Compare July 27, 2023 15:04

pitdicker mentioned this pull request Jul 27, 2023

Make methods on NaiveDate const #1205

Merged

pitdicker force-pushed the fixed_offset_add_impls branch 2 times, most recently from 90c8384 to 6e00637 Compare August 6, 2023 19:27

pitdicker force-pushed the fixed_offset_add_impls branch from 6e00637 to 85a1ebc Compare September 2, 2023 06:29

djc reviewed Sep 5, 2023

View reviewed changes

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

pitdicker mentioned this pull request Sep 8, 2023

Add easy initialization macros #1270

Open

pitdicker force-pushed the fixed_offset_add_impls branch from e74dcba to 8c53477 Compare September 23, 2023 06:04

This was referenced Sep 23, 2023

Add benchmark for DateTime::with_* #1309

Merged

Add NaiveTime::overflowing_(add|sub)_offset #1310

Merged

pitdicker force-pushed the fixed_offset_add_impls branch from 8c53477 to 23d9c3e Compare September 23, 2023 14:22

pitdicker mentioned this pull request Sep 23, 2023

Add NaiveDateTime::checked_(add|sub)_offset #1313

Merged

pitdicker added 2 commits September 25, 2023 13:32

Move Add and Sub impls for Fixedoffset to datetime modules

225cfaa

Use checked_(add|sub)_offset in Add and Sub impls of DateTime

018be06

pitdicker force-pushed the fixed_offset_add_impls branch from 23d9c3e to 018be06 Compare September 25, 2023 11:32

pitdicker commented Sep 25, 2023

View reviewed changes

djc approved these changes Sep 25, 2023

View reviewed changes

src/datetime/mod.rs

}

}

impl<Tz: TimeZone> Add<FixedOffset> for DateTime<Tz> {

Copy link

Member

djc Sep 25, 2023

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 merged commit d450b68 into chronotope:0.4.x Sep 25, 2023

pitdicker deleted the fixed_offset_add_impls branch September 25, 2023 12:18

This was referenced Jan 24, 2024

Bump chrono from 0.4.31 to 0.4.32 ISibboI/typed-session#27

Closed

Bump chrono from 0.4.31 to 0.4.32 ISibboI/vocabulary-learning-application#115

Closed

This was referenced Feb 2, 2024

Debug impl of DateTime can panic #1047

Closed

Recognize our review capacity is limited for non-trivial work #1417

Open

Conversation

pitdicker commented May 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

jtmoon79 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

pitdicker commented May 23, 2023

Uh oh!

pitdicker commented Jun 1, 2023

Uh oh!

djc commented Jun 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pitdicker commented Aug 11, 2023

Uh oh!

codecov bot commented Sep 2, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

djc left a comment

Choose a reason for hiding this comment

Uh oh!

djc Aug 7, 2023

Choose a reason for hiding this comment

Uh oh!

pitdicker Sep 5, 2023

Choose a reason for hiding this comment

Uh oh!

pitdicker commented Sep 12, 2023

Uh oh!

djc commented Sep 13, 2023

Uh oh!

pitdicker commented Sep 13, 2023

Uh oh!

pitdicker commented Sep 23, 2023

Uh oh!

djc commented Sep 23, 2023

Uh oh!

pitdicker commented Sep 25, 2023

Uh oh!

pitdicker Sep 25, 2023

Choose a reason for hiding this comment

Uh oh!

djc Sep 25, 2023

Choose a reason for hiding this comment

Uh oh!

pitdicker Sep 25, 2023

Choose a reason for hiding this comment

Uh oh!

djc Sep 25, 2023

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

pitdicker commented May 18, 2023 •

edited

Loading

jtmoon79 left a comment •

edited

Loading

djc commented Jun 1, 2023 •

edited

Loading

codecov bot commented Sep 2, 2023 •

edited

Loading