-
Notifications
You must be signed in to change notification settings - Fork 517
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
Conversation
92dc230
to
f4012de
Compare
src/naive/internals.rs
Outdated
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 |
There was a problem hiding this comment.
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.
src/naive/isoweek.rs
Outdated
@@ -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) } |
There was a problem hiding this comment.
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.
src/naive/internals.rs
Outdated
pub(super) const MIN_YEAR: DateImpl = DateImpl::new(i32::MIN >> 13); | ||
|
||
impl DateImpl { | ||
pub(super) const fn new(val: i32) -> Self { |
There was a problem hiding this comment.
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]
.
While working on #1043 and now I feel we have abstractions in the wrong place here. The modules @djc What do you think of the following?
|
Splitting up 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 By comparison, |
👍 I'll give it a try and and we will know soon enough if it works well. |
Sorry, I missed this sentence. The |
f4012de
to
1127adf
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
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. |
Rewritten on top of #1212. Still keeping as draft until that refactor is completed. |
1127adf
to
a796272
Compare
@@ -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 |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 { |
There was a problem hiding this comment.
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.
a796272
to
1684fee
Compare
🎉Thank you! That were a lot of PRs to go through. |
NonZeroI32
is ugly to work with. But I really like what it brings:NaiveDate
,NaiveDateTime
andDateTime
take up te same size when wrapped inOption
.We can easily make it a
NonZeroI32
because aNaiveDate
consists of a year, ordinal and flags. The ordinal is always in the range1..=366
, and the flags are also never0
.I considered if switching the
Of
type toNonZero*32
would make things easier. But we do most calculations on that type, and it quickly gets unergonomic (I tried).