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

Use NonZeroI32 inside NaiveDate #1207

Merged
merged 2 commits into from
Feb 12, 2024
Merged

Conversation

pitdicker
Copy link
Collaborator

@pitdicker pitdicker commented Jul 27, 2023

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 Outdated Show resolved Hide resolved
@pitdicker pitdicker force-pushed the nonzero_naivedate branch 3 times, most recently from 92dc230 to f4012de Compare July 28, 2023 08:19
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
Copy link
Member

Choose a reason for hiding this comment

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

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

@@ -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) }
Copy link
Member

Choose a reason for hiding this comment

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

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

pub(super) const MIN_YEAR: DateImpl = DateImpl::new(i32::MIN >> 13);

impl DateImpl {
pub(super) const fn new(val: i32) -> Self {
Copy link
Member

Choose a reason for hiding this comment

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

Maybe also make these methods #[inline].

@pitdicker
Copy link
Collaborator Author

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
Copy link
Member

djc commented Jul 29, 2023

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
Copy link
Collaborator Author

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

@pitdicker
Copy link
Collaborator Author

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.

Copy link

codecov bot commented Feb 11, 2024

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
Copy link
Collaborator Author

pitdicker commented Feb 11, 2024

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

@pitdicker pitdicker marked this pull request as ready for review February 12, 2024 15:33
@@ -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
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

@pitdicker pitdicker Feb 12, 2024

Choose a reason for hiding this comment

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

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.

}

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

Choose a reason for hiding this comment

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

I think we should #[inline] this.

@pitdicker pitdicker merged commit 0b4d5ff into chronotope:main Feb 12, 2024
37 checks passed
@pitdicker pitdicker deleted the nonzero_naivedate branch February 12, 2024 16:05
@pitdicker
Copy link
Collaborator Author

🎉

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

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.

2 participants