From 0dd1d3fdfa7058a108b3f4044e4311ee561b1138 Mon Sep 17 00:00:00 2001 From: Hiroaki Yutani Date: Mon, 1 Apr 2024 08:36:23 +0900 Subject: [PATCH] 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 --- src/lib.rs | 3 +++ src/sexp/complex.rs | 17 +++++++++++------ src/sexp/integer.rs | 17 +++++++++++------ src/sexp/logical.rs | 15 ++++++++++----- src/sexp/real.rs | 17 +++++++++++------ src/sexp/string.rs | 43 +++++++++++++++++++++++++------------------ 6 files changed, 71 insertions(+), 41 deletions(-) diff --git a/src/lib.rs b/src/lib.rs index 6f59fb2a..b151bcdc 100644 --- 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 index a4750dc8..8bd95bb0 100644 --- 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 index cc604907..a4746e30 100644 --- 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 index 05e79369..6c8ddcf3 100644 --- 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 index 1ea95e97..155d99ad 100644 --- 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 index fa9ac442..61c5c9a8 100644 --- 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 { - // 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, + ) + }) + } } }