Some followup to #145 (#146)

* Set #![warn(unsafe_op_in_unsafe_fn)] * Fix unsafe_op_in_unsafe_fn warnings * Add set_elt_unchecked() for completeness * Use isize for the interface * Add comments why we shouldn't use set_elt_unchecked() here
yutannihilation · Mar 31, 2024 · 0dd1d3f · 0dd1d3f
1 parent 0b45c86
commit 0dd1d3f
Show file tree

Hide file tree

Showing 6 changed files with 71 additions and 41 deletions.
diff --git a/src/lib.rs b/src/lib.rs
@@ -1,3 +1,6 @@
+// TODO: Remove this when edition is set to 2024
+#![warn(unsafe_op_in_unsafe_fn)]
+
 //! # Savvy - A Simple R Interface
 //!
 //! **savvy** is a simple R extension interface using Rust, like the [extendr] framework.

diff --git a/src/sexp/complex.rs b/src/sexp/complex.rs
@@ -66,20 +66,21 @@ impl OwnedComplexSexp {
     pub fn set_elt(&mut self, i: usize, v: Complex64) -> crate::error::Result<()> {
         super::utils::assert_len(self.len, i)?;
 
-        unsafe {
-            *(self.raw.add(i)) = v;
-        }
+        unsafe { self.set_elt_unchecked(i, v) };
 
         Ok(())
     }
 
+    #[inline]
+    unsafe fn set_elt_unchecked(&mut self, i: usize, v: Complex64) {
+        unsafe { *(self.raw.add(i)) = v };
+    }
+
     /// Set the `i`-th element to NA.
     pub fn set_na(&mut self, i: usize) -> crate::error::Result<()> {
         super::utils::assert_len(self.len, i)?;
 
-        unsafe {
-            *(self.raw.add(i)) = Complex64::na();
-        }
+        unsafe { self.set_elt_unchecked(i, Complex64::na()) };
 
         Ok(())
     }
@@ -144,7 +145,11 @@ impl OwnedComplexSexp {
 
                 let mut last_index = 0;
                 for (i, v) in iter.enumerate() {
+                    // The upper bound of size_hint() is just for optimization
+                    // and what we should not trust. So, we should't use
+                    // `set_elt_unchecked()` here.
                     out.set_elt(i, v)?;
+
                     last_index = i;
                 }
 

diff --git a/src/sexp/integer.rs b/src/sexp/integer.rs
@@ -68,20 +68,21 @@ impl OwnedIntegerSexp {
     pub fn set_elt(&mut self, i: usize, v: i32) -> crate::error::Result<()> {
         super::utils::assert_len(self.len, i)?;
 
-        unsafe {
-            *(self.raw.add(i)) = v;
-        }
+        unsafe { self.set_elt_unchecked(i, v) };
 
         Ok(())
     }
 
+    #[inline]
+    unsafe fn set_elt_unchecked(&mut self, i: usize, v: i32) {
+        unsafe { *(self.raw.add(i)) = v };
+    }
+
     /// Set the `i`-th element to NA.
     pub fn set_na(&mut self, i: usize) -> crate::error::Result<()> {
         super::utils::assert_len(self.len, i)?;
 
-        unsafe {
-            *(self.raw.add(i)) = i32::na();
-        }
+        unsafe { self.set_elt_unchecked(i, i32::na()) };
 
         Ok(())
     }
@@ -173,7 +174,11 @@ impl OwnedIntegerSexp {
 
                 let mut last_index = 0;
                 for (i, v) in iter.enumerate() {
+                    // The upper bound of size_hint() is just for optimization
+                    // and what we should not trust. So, we should't use
+                    // `set_elt_unchecked()` here.
                     out.set_elt(i, v)?;
+
                     last_index = i;
                 }
 

diff --git a/src/sexp/logical.rs b/src/sexp/logical.rs
@@ -63,22 +63,23 @@ impl OwnedLogicalSexp {
     pub fn set_elt(&mut self, i: usize, v: bool) -> crate::error::Result<()> {
         super::utils::assert_len(self.len, i)?;
 
-        unsafe { self.set_elt_unchecked(i as _, v as _) };
+        unsafe { self.set_elt_unchecked(i, v as _) };
 
         Ok(())
     }
 
     // Set the value of the `i`-th element.
     // Safety: the user has to assure bounds are checked.
-    pub(crate) unsafe fn set_elt_unchecked(&mut self, i: isize, v: i32) {
-        SET_LOGICAL_ELT(self.inner, i, v);
+    #[inline]
+    unsafe fn set_elt_unchecked(&mut self, i: usize, v: i32) {
+        unsafe { SET_LOGICAL_ELT(self.inner, i as _, v) };
     }
 
     /// Set the `i`-th element to NA.
     pub fn set_na(&mut self, i: usize) -> crate::error::Result<()> {
         super::utils::assert_len(self.len, i)?;
 
-        unsafe { self.set_elt_unchecked(i as _, R_NaInt) };
+        unsafe { self.set_elt_unchecked(i, R_NaInt) };
 
         Ok(())
     }
@@ -169,7 +170,11 @@ impl OwnedLogicalSexp {
 
                 let mut last_index = 0;
                 for (i, v) in iter.enumerate() {
+                    // The upper bound of size_hint() is just for optimization
+                    // and what we should not trust. So, we should't use
+                    // `set_elt_unchecked()` here.
                     out.set_elt(i, v)?;
+
                     last_index = i;
                 }
 
@@ -199,7 +204,7 @@ impl OwnedLogicalSexp {
         let mut out = unsafe { Self::new_without_init(x_slice.len())? };
         for (i, v) in x_slice.iter().enumerate() {
             // Safety: slice and OwnedLogicalSexp have the same length.
-            unsafe { out.set_elt_unchecked(i as _, *v as _) };
+            unsafe { out.set_elt_unchecked(i, *v as _) };
         }
         Ok(out)
     }

diff --git a/src/sexp/real.rs b/src/sexp/real.rs
@@ -64,20 +64,21 @@ impl OwnedRealSexp {
     pub fn set_elt(&mut self, i: usize, v: f64) -> crate::error::Result<()> {
         super::utils::assert_len(self.len, i)?;
 
-        unsafe {
-            *(self.raw.add(i)) = v;
-        }
+        unsafe { self.set_elt_unchecked(i, v) };
 
         Ok(())
     }
 
+    #[inline]
+    unsafe fn set_elt_unchecked(&mut self, i: usize, v: f64) {
+        unsafe { *(self.raw.add(i)) = v };
+    }
+
     /// Set the `i`-th element to NA.
     pub fn set_na(&mut self, i: usize) -> crate::error::Result<()> {
         super::utils::assert_len(self.len, i)?;
 
-        unsafe {
-            *(self.raw.add(i)) = f64::na();
-        }
+        unsafe { self.set_elt_unchecked(i, f64::na()) };
 
         Ok(())
     }
@@ -169,7 +170,11 @@ impl OwnedRealSexp {
 
                 let mut last_index = 0;
                 for (i, v) in iter.enumerate() {
+                    // The upper bound of size_hint() is just for optimization
+                    // and what we should not trust. So, we should't use
+                    // `set_elt_unchecked()` here.
                     out.set_elt(i, v)?;
+
                     last_index = i;
                 }
 

diff --git a/src/sexp/string.rs b/src/sexp/string.rs
@@ -63,15 +63,16 @@ impl OwnedStringSexp {
                 self.len, i
             )));
         }
-        unsafe { self.set_elt_unchecked(i as _, str_to_charsxp(v)?) };
+        unsafe { self.set_elt_unchecked(i, str_to_charsxp(v)?) };
 
         Ok(())
     }
 
     // Set the value of the `i`-th element.
     // Safety: the user has to assure bounds are checked.
-    pub(crate) unsafe fn set_elt_unchecked(&mut self, i: isize, v: SEXP) {
-        SET_STRING_ELT(self.inner, i, v);
+    #[inline]
+    unsafe fn set_elt_unchecked(&mut self, i: usize, v: SEXP) {
+        unsafe { SET_STRING_ELT(self.inner, i as _, v) };
     }
 
     /// Set the `i`-th element to NA.
@@ -83,7 +84,7 @@ impl OwnedStringSexp {
             )));
         }
 
-        unsafe { self.set_elt_unchecked(i as _, R_NaString) };
+        unsafe { self.set_elt_unchecked(i, R_NaString) };
 
         Ok(())
     }
@@ -131,7 +132,11 @@ impl OwnedStringSexp {
 
                 let mut last_index = 0;
                 for (i, v) in iter.enumerate() {
+                    // The upper bound of size_hint() is just for optimization
+                    // and what we should not trust. So, we should't use
+                    // `set_elt_unchecked()` here.
                     out.set_elt(i, v.as_ref())?;
+
                     last_index = i;
                 }
 
@@ -162,7 +167,7 @@ impl OwnedStringSexp {
         let mut out = Self::new(x_slice.len())?;
         for (i, v) in x_slice.iter().enumerate() {
             // Safety: slice and OwnedStringSexp have the same length.
-            unsafe { out.set_elt_unchecked(i as _, str_to_charsxp(v.as_ref())?) };
+            unsafe { out.set_elt_unchecked(i, str_to_charsxp(v.as_ref())?) };
         }
         Ok(out)
     }
@@ -183,19 +188,21 @@ impl OwnedStringSexp {
 }
 
 unsafe fn str_to_charsxp(v: &str) -> crate::error::Result<SEXP> {
-    // We might be able to put `R_NaString` directly without using
-    // <&str>::na(), but probably this is an inevitable cost of
-    // providing <&str>::na().
-    if v.is_na() {
-        Ok(savvy_ffi::R_NaString)
-    } else {
-        crate::unwind_protect(|| {
-            Rf_mkCharLenCE(
-                v.as_ptr() as *const c_char,
-                v.len() as i32,
-                cetype_t_CE_UTF8,
-            )
-        })
+    unsafe {
+        // We might be able to put `R_NaString` directly without using
+        // <&str>::na(), but probably this is an inevitable cost of
+        // providing <&str>::na().
+        if v.is_na() {
+            Ok(savvy_ffi::R_NaString)
+        } else {
+            crate::unwind_protect(|| {
+                Rf_mkCharLenCE(
+                    v.as_ptr() as *const c_char,
+                    v.len() as i32,
+                    cetype_t_CE_UTF8,
+                )
+            })
+        }
     }
 }