Use `NonZeroI32` inside `NaiveDate` #1207

pitdicker · 2023-07-27T17:38:50Z

NonZeroI32 is ugly to work with. But I really like what it brings: NaiveDate, NaiveDateTime and DateTime take up te same size when wrapped in Option.

We can easily make it a NonZeroI32 because a NaiveDate consists of a year, ordinal and flags. The ordinal is always in the range 1..=366, and the flags are also never 0.

I considered if switching the Of type to NonZero*32 would make things easier. But we do most calculations on that type, and it quickly gets unergonomic (I tried).

src/naive/internals.rs

djc · 2023-07-28T08:54:44Z

src/naive/internals.rs

-pub(super) type DateImpl = i32;
+#[derive(Clone, Copy, Debug, Eq, Hash, PartialEq, PartialOrd, Ord)]
+#[cfg_attr(feature = "rkyv", derive(Archive, Deserialize, Serialize))]
+#[allow(unreachable_pub)] // must be pub for rkyv


// must be pub for rkyv seems surprising, why is this? There should really be a way for private types to be Archive etc.

djc · 2023-07-28T08:55:46Z

src/naive/isoweek.rs

@@ -47,7  47,7 @@ pub(super) fn iso_week_from_yof(year: i32, of: Of) -> IsoWeek {
        }
    };
    let flags = YearFlags::from_year(year);
-    IsoWeek { ywf: (year << 10) | (week << 4) as DateImpl | DateImpl::from(flags.0) }
+    IsoWeek { ywf: DateImpl::new((year << 10) | (week << 4) as i32 | flags.0 as i32) }


Maybe we need DateImpl::from_yw() or DateImpl::from_yof()? Looks like we have a bunch of repetition of similar patterns.

djc · 2023-07-28T08:58:01Z

src/naive/internals.rs

+pub(super) const MIN_YEAR: DateImpl = DateImpl::new(i32::MIN >> 13);
+
+impl DateImpl {
+    pub(super) const fn new(val: i32) -> Self {


Maybe also make these methods #[inline].

pitdicker · 2023-07-28T17:06:38Z

While working on #1043 and now I feel we have abstractions in the wrong place here.

The modules naive::date and naive::internals don't have a clear separation. In several cases methods on NaiveDate directly work on the internal i32. And this is entirely reasonable to implement some operations efficiently.

@djc What do you think of the following?

Split naive::date into multiple files like naive::time and naive::datetime.
Merge naive::internals into naive::date
Remove the DateImpl type (use NaiveDate instead)
Maybe: move Of and Mdf into a separate modules to enforce a clean abstraction
Simplify whatever this makes easier.

djc · 2023-07-29T13:48:39Z

Splitting up naive::date similar to datetime and time is great, let's do that first in a separate PR?

I think your plan makes sense conceptually, I'd like to see how module sizes work out before we (mostly you 😄 ) work through all of the proposed changes. From a quick look, moving NaiveDate itself and its trait implementations into a separate module results in a module that's well over 2000 lines of code, so I don't think merging (currently at ~900 lines) into it would be a great idea.

By comparison, internals is at 900 lines which is largish for my taste but 400 of those are tests, so it's pretty good IMO. Inline modules might make sense for Of and Mdf though.

pitdicker · 2023-07-29T14:15:35Z

👍 I'll give it a try and and we will know soon enough if it works well.

pitdicker · 2023-08-02T11:29:42Z

I'd like to see how module sizes work out before we (mostly you 😄 ) work through all of the proposed changes.

Sorry, I missed this sentence.

The naive::date module did not really increase in size in #1212. Folding the functionality of Of into NaiveDates takes only a couple of lines extra. And in the end I moved the Mdf, YearFlags and NaiveWeek types to modules of their own.

codecov · 2024-02-11T15:41:49Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (d9d6b63) 91.84% compared to head (1684fee) 91.85%.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #1207    /-   ##
=======================================
  Coverage   91.84%   91.85%           
=======================================
  Files          40       40           
  Lines       17450    17469    19     
=======================================
  Hits        16027    16046    19     
  Misses       1423     1423

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

pitdicker · 2024-02-11T15:48:43Z

Rewritten on top of #1212. Still keeping as draft until that refactor is completed.

djc · 2024-02-12T15:38:00Z

src/naive/date/mod.rs

@@ -99,7  100,7 @@ mod tests;
 )]
 #[cfg_attr(feature = "rkyv-validation", archive(check_bytes))]
 pub struct NaiveDate {
-    yof: i32, // (year << 13) | of
+    yof: NonZeroI32, // (year << 13) | of


Does this change the NaiveDate MAX or MIN? If not, why not?

This doesn't change that much.
Our internal representation uses an ordinal that is always in the range 1..366, and year flags that are never 0. So the value was already never 0; we now tell the type system as much.

djc · 2024-02-12T15:47:13Z

src/naive/date/mod.rs

+    }
+
+    /// Get the raw year-ordinal-flags `i32`.
+    const fn yof(&self) -> i32 {


I think we should #[inline] this.

pitdicker · 2024-02-12T16:08:41Z

🎉

Thank you! That were a lot of PRs to go through.

djc reviewed Jul 28, 2023

View reviewed changes

src/naive/internals.rs Outdated Show resolved Hide resolved

pitdicker force-pushed the nonzero_naivedate branch 3 times, most recently from 92dc230 to f4012de Compare July 28, 2023 08:19

djc reviewed Jul 28, 2023

View reviewed changes

pitdicker mentioned this pull request Aug 2, 2023

Refactor internals module #1212

Closed

pitdicker marked this pull request as draft August 2, 2023 13:32

pitdicker mentioned this pull request Sep 27, 2023

0.5 release planning #970

Open

pitdicker mentioned this pull request Feb 7, 2024

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

Open

pitdicker force-pushed the nonzero_naivedate branch from f4012de to 1127adf Compare February 11, 2024 15:39

pitdicker force-pushed the nonzero_naivedate branch from 1127adf to a796272 Compare February 12, 2024 15:32

pitdicker marked this pull request as ready for review February 12, 2024 15:33

djc reviewed Feb 12, 2024

View reviewed changes

djc approved these changes Feb 12, 2024

View reviewed changes

pitdicker added 2 commits February 12, 2024 16:52

Add test for the size of chrono's main data types

95e2439

Use NonZeroI32 inside NaiveDate

1684fee

pitdicker force-pushed the nonzero_naivedate branch from a796272 to 1684fee Compare February 12, 2024 15:53

pitdicker merged commit 0b4d5ff into chronotope:main Feb 12, 2024
37 checks passed

pitdicker deleted the nonzero_naivedate branch February 12, 2024 16:05

This was referenced Feb 13, 2024

Mdf performance and docs #1438

Merged

0.5: Convert API to return Result: naive module #1445

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use `NonZeroI32` inside `NaiveDate` #1207

Use `NonZeroI32` inside `NaiveDate` #1207

pitdicker commented Jul 27, 2023 •

edited

Loading

djc Jul 28, 2023

djc Jul 28, 2023

djc Jul 28, 2023

pitdicker commented Jul 28, 2023

djc commented Jul 29, 2023 •

edited

Loading

pitdicker commented Jul 29, 2023

pitdicker commented Aug 2, 2023

codecov bot commented Feb 11, 2024 •

edited

Loading

pitdicker commented Feb 11, 2024 •

edited

Loading

djc Feb 12, 2024

pitdicker Feb 12, 2024 •

edited

Loading

djc Feb 12, 2024

pitdicker commented Feb 12, 2024

Use NonZeroI32 inside NaiveDate #1207

Use NonZeroI32 inside NaiveDate #1207

Conversation

pitdicker commented Jul 27, 2023 • edited Loading

djc Jul 28, 2023

Choose a reason for hiding this comment

djc Jul 28, 2023

Choose a reason for hiding this comment

djc Jul 28, 2023

Choose a reason for hiding this comment

pitdicker commented Jul 28, 2023

djc commented Jul 29, 2023 • edited Loading

pitdicker commented Jul 29, 2023

pitdicker commented Aug 2, 2023

codecov bot commented Feb 11, 2024 • edited Loading

Codecov Report

pitdicker commented Feb 11, 2024 • edited Loading

djc Feb 12, 2024

Choose a reason for hiding this comment

pitdicker Feb 12, 2024 • edited Loading

Choose a reason for hiding this comment

djc Feb 12, 2024

Choose a reason for hiding this comment

pitdicker commented Feb 12, 2024

🎉

Use `NonZeroI32` inside `NaiveDate` #1207

Use `NonZeroI32` inside `NaiveDate` #1207

pitdicker commented Jul 27, 2023 •

edited

Loading

djc commented Jul 29, 2023 •

edited

Loading

codecov bot commented Feb 11, 2024 •

edited

Loading

pitdicker commented Feb 11, 2024 •

edited

Loading

pitdicker Feb 12, 2024 •

edited

Loading