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

Make new unchecked unsafe #72

Open
Toorero opened this issue Dec 6, 2024 · 3 comments · May be fixed by #73
Open

Make new unchecked unsafe #72

Toorero opened this issue Dec 6, 2024 · 3 comments · May be fixed by #73

Comments

@Toorero
Copy link

Toorero commented Dec 6, 2024

I was just reading your code and was wondering what's up with the Day::__new_unchecked function.

Wouldn't it make more sense to "really" expose it publicly and make it unsafe, so that the caller has to take care to uphold safety?

diff --git a/src/template/day.rs b/src/template/day.rs
--- a/src/template/day.rs
+++ b/src/template/day.rs
@@ -31,9 +31,12 @@ impl Day {
         Some(Self(day))
     }
 
-    // Not part of the public API
-    #[doc(hidden)]
-    pub const fn __new_unchecked(day: u8) -> Self {
+    /// Creates a new day without checking if it's in the valid range.
+    ///
+    /// # Safety
+    ///
+    /// Ensure that day is in range `1..=24`.
+    pub const unsafe fn new_unchecked(day: u8) -> Self {
         Self(day)
     }
 
@@ -146,7 +149,7 @@ macro_rules! day {
                 "`, expecting a value between 1 and 25"
             ),
         );
-        $crate::template::Day::__new_unchecked($day)
+        unsafe { $crate::template::Day::new_unchecked($day) }
     }};
 }
@fspoettel
Copy link
Owner

I'm not 100% sure anymore, maybe @tguichaoua remembers why this was added in #35?

@tguichaoua
Copy link
Contributor

tguichaoua commented Dec 6, 2024

Back when it was added it was impossible to do comparison in a const function, so to create a Day in a const context I implemeted it with a day! macros that check if the value is a valid day then pass it to new_unchecked.
Since it's now possible to do comparison in a const function, we can remove new_unchecked and make new const and day! may be changed to something like this (note that Option::except isn't const):

macro_rules! day {
    ($day:expr) => {
        const {
            let day: u8 = $day;
            match Day::new(day) {
                Some(day) => day,
                None => panic!("invalid day number `{day}`, expecting a value between 1 and 25"),
            }
        }
    };
}

Also unsafe wouldn't be suitable here as Day didn't hold any soundness invariant.

@tguichaoua tguichaoua linked a pull request Dec 6, 2024 that will close this issue
@Toorero
Copy link
Author

Toorero commented Dec 18, 2024

Interesting, thanks for the input.

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 a pull request may close this issue.

3 participants