Ensure Of is always valid

pitdicker · pitdicker · commit 6540cbf99a10 · 2023-04-30T09:45:49.000+02:00
diff --git a/src/naive/date.rs b/src/naive/date.rs
@@ -242,19 +242,29 @@ impl NaiveDate {
     pub(crate) fn weeks_from(&self, day: Weekday) -> i32 {
         (self.ordinal() as i32 - self.weekday().num_days_from(day) as i32 + 6) / 7
     }
-    /// Makes a new `NaiveDate` from year and packed ordinal-flags, with a verification.
-    fn from_of(year: i32, of: Of) -> Option<NaiveDate> {
-        if (MIN_YEAR..=MAX_YEAR).contains(&year) && of.valid() {
-            let Of(of) = of;
-            Some(NaiveDate { ymdf: (year << 13) | (of as DateImpl) })
-        } else {
-            None
+
+    /// Makes a new `NaiveDate` from year, ordinal and flags.
+    /// Does not check whether the flags are correct for the provided year.
+    const fn from_of(year: i32, ordinal: u32, flags: YearFlags) -> Option<NaiveDate> {
+        if year < MIN_YEAR || year > MAX_YEAR {
+            return None; // Out-of-range
+        }
+        match Of::new(ordinal, flags) {
+            Some(of) => Some(NaiveDate { ymdf: (year << 13) | (of.inner() as DateImpl) }),
+            None => None, // Invalid: Ordinal outside of the nr of days in a year with those flags.
         }
     }
 
-    /// Makes a new `NaiveDate` from year and packed month-day-flags, with a verification.
-    fn from_mdf(year: i32, mdf: Mdf) -> Option<NaiveDate> {
-        NaiveDate::from_of(year, mdf.to_of())
+    /// Makes a new `NaiveDate` from year and packed month-day-flags.
+    /// Does not check whether the flags are correct for the provided year.
+    const fn from_mdf(year: i32, mdf: Mdf) -> Option<NaiveDate> {
+        if year < MIN_YEAR || year > MAX_YEAR {
+            return None; // Out-of-range
+        }
+        match mdf.to_of() {
+            Some(of) => Some(NaiveDate { ymdf: (year << 13) | (of.inner() as DateImpl) }),
+            None => None, // Non-existing date
+        }
     }
 
     /// Makes a new `NaiveDate` from the [calendar date](#calendar-date)
@@ -325,7 +335,7 @@ impl NaiveDate {
     #[must_use]
     pub fn from_yo_opt(year: i32, ordinal: u32) -> Option<NaiveDate> {
         let flags = YearFlags::from_year(year);
-        NaiveDate::from_of(year, Of::new(ordinal, flags)?)
+        NaiveDate::from_of(year, ordinal, flags)
     }
 
     /// Makes a new `NaiveDate` from the [ISO week date](#week-date)
@@ -394,20 +404,17 @@ impl NaiveDate {
             if weekord <= delta {
                 // ordinal < 1, previous year
                 let prevflags = YearFlags::from_year(year - 1);
-                NaiveDate::from_of(
-                    year - 1,
-                    Of::new(weekord + prevflags.ndays() - delta, prevflags)?,
-                )
+                NaiveDate::from_of(year - 1, weekord + prevflags.ndays() - delta, prevflags)
             } else {
                 let ordinal = weekord - delta;
                 let ndays = flags.ndays();
                 if ordinal <= ndays {
                     // this year
-                    NaiveDate::from_of(year, Of::new(ordinal, flags)?)
+                    NaiveDate::from_of(year, ordinal, flags)
                 } else {
                     // ordinal > ndays, next year
                     let nextflags = YearFlags::from_year(year + 1);
-                    NaiveDate::from_of(year + 1, Of::new(ordinal - ndays, nextflags)?)
+                    NaiveDate::from_of(year + 1, ordinal - ndays, nextflags)
                 }
             }
         } else {
@@ -452,7 +459,7 @@ impl NaiveDate {
         let (year_div_400, cycle) = div_mod_floor(days, 146_097);
         let (year_mod_400, ordinal) = internals::cycle_to_yo(cycle as u32);
         let flags = YearFlags::from_year_mod_400(year_mod_400 as i32);
-        NaiveDate::from_of(year_div_400 * 400 + year_mod_400 as i32, Of::new(ordinal, flags)?)
+        NaiveDate::from_of(year_div_400 * 400 + year_mod_400 as i32, ordinal, flags)
     }
 
     /// Makes a new `NaiveDate` by counting the number of occurrences of a particular day-of-week
@@ -933,20 +940,16 @@ impl NaiveDate {
     /// Returns `None` when the resulting `NaiveDate` would be invalid.
     #[inline]
     fn with_mdf(&self, mdf: Mdf) -> Option<NaiveDate> {
-        self.with_of(mdf.to_of())
+        Some(self.with_of(mdf.to_of()?))
     }
 
     /// Makes a new `NaiveDate` with the packed ordinal-flags changed.
     ///
     /// Returns `None` when the resulting `NaiveDate` would be invalid.
+    /// Does not check if the year flags match the year.
     #[inline]
-    fn with_of(&self, of: Of) -> Option<NaiveDate> {
-        if of.valid() {
-            let Of(of) = of;
-            Some(NaiveDate { ymdf: (self.ymdf & !0b1_1111_1111_1111) | of as DateImpl })
-        } else {
-            None
-        }
+    const fn with_of(&self, of: Of) -> NaiveDate {
+        NaiveDate { ymdf: (self.ymdf & !0b1_1111_1111_1111) | of.inner() as DateImpl }
     }
 
     /// Makes a new `NaiveDate` for the next calendar date.
@@ -975,7 +978,10 @@ impl NaiveDate {
     #[inline]
     #[must_use]
     pub fn succ_opt(&self) -> Option<NaiveDate> {
-        self.with_of(self.of().succ()).or_else(|| NaiveDate::from_ymd_opt(self.year() + 1, 1, 1))
+        match self.of().succ() {
+            Some(of) => Some(self.with_of(of)),
+            None => NaiveDate::from_ymd_opt(self.year() + 1, 1, 1),
+        }
     }
 
     /// Makes a new `NaiveDate` for the previous calendar date.
@@ -1004,7 +1010,10 @@ impl NaiveDate {
     #[inline]
     #[must_use]
     pub fn pred_opt(&self) -> Option<NaiveDate> {
-        self.with_of(self.of().pred()).or_else(|| NaiveDate::from_ymd_opt(self.year() - 1, 12, 31))
+        match self.of().pred() {
+            Some(of) => Some(self.with_of(of)),
+            None => NaiveDate::from_ymd_opt(self.year() - 1, 12, 31),
+        }
     }
 
     /// Adds the `days` part of given `Duration` to the current date.
@@ -1036,7 +1045,7 @@ impl NaiveDate {
 
         let (year_mod_400, ordinal) = internals::cycle_to_yo(cycle as u32);
         let flags = YearFlags::from_year_mod_400(year_mod_400 as i32);
-        NaiveDate::from_of(year_div_400 * 400 + year_mod_400 as i32, Of::new(ordinal, flags)?)
+        NaiveDate::from_of(year_div_400 * 400 + year_mod_400 as i32, ordinal, flags)
     }
 
     /// Subtracts the `days` part of given `Duration` from the current date.
@@ -1068,7 +1077,7 @@ impl NaiveDate {
 
         let (year_mod_400, ordinal) = internals::cycle_to_yo(cycle as u32);
         let flags = YearFlags::from_year_mod_400(year_mod_400 as i32);
-        NaiveDate::from_of(year_div_400 * 400 + year_mod_400 as i32, Of::new(ordinal, flags)?)
+        NaiveDate::from_of(year_div_400 * 400 + year_mod_400 as i32, ordinal, flags)
     }
 
     /// Subtracts another `NaiveDate` from the current date.
@@ -1623,7 +1632,7 @@ impl Datelike for NaiveDate {
     /// ```
     #[inline]
     fn with_ordinal(&self, ordinal: u32) -> Option<NaiveDate> {
-        self.with_of(self.of().with_ordinal(ordinal)?)
+        self.of().with_ordinal(ordinal).map(|of| self.with_of(of))
     }
 
     /// Makes a new `NaiveDate` with the day of year (starting from 0) changed.
@@ -1648,7 +1657,7 @@ impl Datelike for NaiveDate {
     #[inline]
     fn with_ordinal0(&self, ordinal0: u32) -> Option<NaiveDate> {
         let ordinal = ordinal0.checked_add(1)?;
-        self.with_of(self.of().with_ordinal(ordinal)?)
+        self.with_ordinal(ordinal)
     }
 }
 
diff --git a/src/naive/internals.rs b/src/naive/internals.rs
@@ -172,8 +172,9 @@ impl fmt::Debug for YearFlags {
     }
 }
 
+// OL: (ordinal << 1) | leap year flag
 pub(super) const MIN_OL: u32 = 1 << 1;
-pub(super) const MAX_OL: u32 = 366 << 1; // larger than the non-leap last day `(365 << 1) | 1`
+pub(super) const MAX_OL: u32 = 366 << 1; // `(366 << 1) | 1` would be day 366 in a non-leap year
 pub(super) const MAX_MDL: u32 = (12 << 6) | (31 << 1) | 1;
 
 const XX: i8 = -128;
@@ -271,51 +272,56 @@ pub(super) struct Of(pub(crate) u32);
 impl Of {
     #[inline]
     pub(super) const fn new(ordinal: u32, YearFlags(flags): YearFlags) -> Option<Of> {
-        match ordinal <= 366 {
-            true => Some(Of((ordinal << 4) | flags as u32)),
-            false => None,
-        }
+        let of = Of((ordinal << 4) | flags as u32);
+        of.validate()
     }
 
     #[inline]
-    pub(super) const fn from_mdf(Mdf(mdf): Mdf) -> Of {
+    pub(super) const fn from_mdf(Mdf(mdf): Mdf) -> Option<Of> {
         let mdl = mdf >> 3;
-        if mdl <= MAX_MDL {
-            let v = MDL_TO_OL[mdl as usize];
-            Of(mdf.wrapping_sub((v as i32 as u32 & 0x3ff) << 3))
-        } else {
-            // Panicking here would be reasonable, but we are just going on with a safe value.
-            Of(0)
+        if mdl > MAX_MDL {
+            // Panicking on out-of-bounds indexing would be reasonable, but just return `None`.
+            return None;
         }
+        let v = MDL_TO_OL[mdl as usize];
+        let of = Of(mdf.wrapping_sub((v as i32 as u32 & 0x3ff) << 3));
+        of.validate()
     }
 
     #[inline]
-    pub(super) const fn valid(&self) -> bool {
-        let Of(of) = *self;
-        let ol = of >> 3;
-        ol >= MIN_OL && ol <= MAX_OL
+    pub(super) const fn inner(&self) -> u32 {
+        self.0
     }
 
+    /// Returns `(ordinal << 1) | leap-year-flag`.
     #[inline]
-    pub(super) const fn ordinal(&self) -> u32 {
-        let Of(of) = *self;
-        of >> 4
+    pub(super) const fn ol(&self) -> u32 {
+        self.0 >> 3
     }
 
     #[inline]
-    pub(super) const fn with_ordinal(&self, ordinal: u32) -> Option<Of> {
-        if ordinal > 366 {
-            return None;
+    const fn validate(self) -> Option<Of> {
+        let ol = self.ol();
+        match ol >= MIN_OL && ol <= MAX_OL {
+            true => Some(self),
+            false => None,
         }
+    }
 
-        let Of(of) = *self;
-        Some(Of((of & 0b1111) | (ordinal << 4)))
+    #[inline]
+    pub(super) const fn ordinal(&self) -> u32 {
+        self.0 >> 4
+    }
+
+    #[inline]
+    pub(super) const fn with_ordinal(&self, ordinal: u32) -> Option<Of> {
+        let of = Of((ordinal << 4) | (self.0 & 0b1111));
+        of.validate()
     }
 
     #[inline]
     pub(super) const fn flags(&self) -> YearFlags {
-        let Of(of) = *self;
-        YearFlags((of & 0b1111) as u8)
+        YearFlags((self.0 & 0b1111) as u8)
     }
 
     #[inline]
@@ -338,16 +344,20 @@ impl Of {
         Mdf::from_of(*self)
     }
 
+    /// Returns an `Of` with the next day, or `None` if this is the last day of the year.
     #[inline]
-    pub(super) const fn succ(&self) -> Of {
-        let Of(of) = *self;
-        Of(of + (1 << 4))
+    pub(super) const fn succ(&self) -> Option<Of> {
+        let of = Of(self.0 + (1 << 4));
+        of.validate()
     }
 
+    /// Returns an `Of` with the previous day, or `None` if this is the first day of the year.
     #[inline]
-    pub(super) const fn pred(&self) -> Of {
-        let Of(of) = *self;
-        Of(of - (1 << 4))
+    pub(super) const fn pred(&self) -> Option<Of> {
+        if self.ordinal() == 1 {
+            return None;
+        }
+        Some(Of(self.0 - (1 << 4)))
     }
 }
 
@@ -444,7 +454,7 @@ impl Mdf {
 
     #[cfg_attr(feature = "cargo-clippy", allow(clippy::wrong_self_convention))]
     #[inline]
-    pub(super) const fn to_of(&self) -> Of {
+    pub(super) const fn to_of(&self) -> Option<Of> {
         Of::from_mdf(*self)
     }
 }
@@ -522,7 +532,7 @@ mod tests {
                 };
 
                 assert!(
-                    of.valid() == expected,
+                    of.validate().is_some() == expected,
                     "ordinal {} = {:?} should be {} for dominical year {:?}",
                     ordinal,
                     of,
@@ -642,8 +652,7 @@ mod tests {
     fn test_of_fields() {
         for &flags in FLAGS.iter() {
             for ordinal in range_inclusive(1u32, 366) {
-                let of = Of::new(ordinal, flags).unwrap();
-                if of.valid() {
+                if let Some(of) = Of::new(ordinal, flags) {
                     assert_eq!(of.ordinal(), ordinal);
                 }
             }
@@ -656,14 +665,9 @@ mod tests {
             let of = Of::new(ordinal, flags).unwrap();
 
             for ordinal in range_inclusive(0u32, 1024) {
-                let of = match of.with_ordinal(ordinal) {
-                    Some(of) => of,
-                    None if ordinal > 366 => continue,
-                    None => panic!("failed to create Of with ordinal {}", ordinal),
-                };
-
-                assert_eq!(of.valid(), Of::new(ordinal, flags).unwrap().valid());
-                if of.valid() {
+                let of = of.with_ordinal(ordinal);
+                assert_eq!(of, Of::new(ordinal, flags));
+                if let Some(of) = of {
                     assert_eq!(of.ordinal(), ordinal);
                 }
             }
@@ -788,25 +792,25 @@ mod tests {
     #[test]
     fn test_of_to_mdf() {
         for i in range_inclusive(0u32, 8192) {
-            let of = Of(i);
-            assert_eq!(of.valid(), of.to_mdf().valid());
+            if let Some(of) = Of(i).validate() {
+                assert!(of.to_mdf().valid());
+            }
         }
     }
 
     #[test]
     fn test_mdf_to_of() {
         for i in range_inclusive(0u32, 8192) {
             let mdf = Mdf(i);
-            assert_eq!(mdf.valid(), mdf.to_of().valid());
+            assert_eq!(mdf.valid(), mdf.to_of().is_some());
         }
     }
 
     #[test]
     fn test_of_to_mdf_to_of() {
         for i in range_inclusive(0u32, 8192) {
-            let of = Of(i);
-            if of.valid() {
-                assert_eq!(of, of.to_mdf().to_of());
+            if let Some(of) = Of(i).validate() {
+                assert_eq!(of, of.to_mdf().to_of().unwrap());
             }
         }
     }
@@ -816,7 +820,7 @@ mod tests {
         for i in range_inclusive(0u32, 8192) {
             let mdf = Mdf(i);
             if mdf.valid() {
-                assert_eq!(mdf, mdf.to_of().to_mdf());
+                assert_eq!(mdf, mdf.to_of().unwrap().to_mdf());
             }
         }
     }
