diff --git a/.github/workflows/check-non-api.yaml b/.github/workflows/check-non-api.yaml index 1de37540..f4a1a008 100644 --- a/.github/workflows/check-non-api.yaml +++ b/.github/workflows/check-non-api.yaml @@ -26,7 +26,7 @@ jobs: REGEX=$(Rscript tmp.R) - NON_API=$(grep -R -E "${REGEX}" ./savvy-ffi/src/) + NON_API=$(grep -R -E "${REGEX}" ./savvy-ffi/src/ || true) echo "Check result:" echo @@ -36,4 +36,6 @@ jobs: if [ -n "${NON_API}" ]; then echo 'Found what they call "non-API"!' exit 1 + else + echo "OK!" fi diff --git a/CHANGELOG.md b/CHANGELOG.md index b54f1b78..7e98791d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3,6 +3,10 @@ ## [Unreleased] (ReleaseDate) +### Minor improvements + +* Now savvy no longer uses `SETLENGTH`, which is a so-called "non-API" thing. + ## [v0.6.0] (2024-04-20) ### Breaking changes diff --git a/savvy-ffi/src/lib.rs b/savvy-ffi/src/lib.rs index 1c85e88b..23f78ab6 100644 --- a/savvy-ffi/src/lib.rs +++ b/savvy-ffi/src/lib.rs @@ -69,7 +69,6 @@ extern "C" { // Allocation and attributes extern "C" { pub fn Rf_xlength(arg1: SEXP) -> R_xlen_t; - pub fn SETLENGTH(x: SEXP, v: R_xlen_t); pub fn Rf_allocVector(arg1: SEXPTYPE, arg2: R_xlen_t) -> SEXP; pub fn Rf_install(arg1: *const ::std::os::raw::c_char) -> SEXP; pub fn Rf_getAttrib(arg1: SEXP, arg2: SEXP) -> SEXP; diff --git a/src/sexp/complex.rs b/src/sexp/complex.rs index 0c991f3b..c55d2e05 100644 --- a/src/sexp/complex.rs +++ b/src/sexp/complex.rs @@ -5,7 +5,7 @@ use savvy_ffi::CPLXSXP; use savvy_ffi::{COMPLEX, SEXP}; use super::{impl_common_sexp_ops, impl_common_sexp_ops_owned, utils::assert_len, Sexp}; -use crate::protect; +use crate::protect::{self, local_protect}; use crate::NotAvailableValue; // for na() /// An external SEXP of a complex vector. @@ -198,27 +198,34 @@ impl OwnedComplexSexp { // (e.g. `(0..10).filter(|x| x % 2 == 0)`), so it needs to be // truncated to the actual length at last. - let mut out = unsafe { Self::new_without_init(upper)? }; + let inner = crate::alloc_vector(CPLXSXP, upper as _)?; + local_protect(inner); + let raw = unsafe { COMPLEX(inner) }; 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)?; + // and what we should not trust. + assert_len(upper, i)?; + unsafe { *(raw.add(i)) = v }; last_index = i; } let new_len = last_index + 1; - if new_len != upper { - unsafe { - savvy_ffi::SETLENGTH(out.inner, new_len as _); - } - out.len = new_len; + if new_len == upper { + // If the length is the same as expected, use it as it is. + Self::new_from_raw_sexp(inner, upper) + } else { + // If the length is shorter than expected, re-allocate a new + // SEXP and copy the values into it. + let mut out = unsafe { Self::new_without_init(new_len)? }; + // `raw` is longer than new_len, but the elements over new_len are ignored + let raw_slice = unsafe { std::slice::from_raw_parts(raw, new_len) }; + out.as_mut_slice().copy_from_slice(raw_slice); + + Ok(out) } - - Ok(out) } (_, None) => { // When the length is not known at all, collect() it first. diff --git a/src/sexp/integer.rs b/src/sexp/integer.rs index 3eedea41..ac0f2db4 100644 --- a/src/sexp/integer.rs +++ b/src/sexp/integer.rs @@ -4,7 +4,7 @@ use savvy_ffi::{INTEGER, INTSXP, SEXP}; use super::utils::assert_len; use super::{impl_common_sexp_ops, impl_common_sexp_ops_owned, Sexp}; -use crate::protect; +use crate::protect::{self, local_protect}; use crate::NotAvailableValue; // for na() /// An external SEXP of an integer vector. @@ -284,27 +284,35 @@ impl OwnedIntegerSexp { // (e.g. `(0..10).filter(|x| x % 2 == 0)`), so it needs to be // truncated to the actual length at last. - let mut out = unsafe { Self::new_without_init(upper)? }; + let inner = crate::alloc_vector(INTSXP, upper as _)?; + local_protect(inner); + let raw = unsafe { INTEGER(inner) }; 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)?; + // and what we should not trust. + assert_len(upper, i)?; + unsafe { *(raw.add(i)) = v }; last_index = i; } let new_len = last_index + 1; - if new_len != upper { - unsafe { - savvy_ffi::SETLENGTH(out.inner, new_len as _); - } - out.len = new_len; + if new_len == upper { + // If the length is the same as expected, use it as it is. + Self::new_from_raw_sexp(inner, upper) + } else { + // If the length is shorter than expected, re-allocate a new + // SEXP and copy the values into it. + let out = unsafe { Self::new_without_init(new_len)? }; + let dst = unsafe { std::slice::from_raw_parts_mut(out.raw, new_len) }; + // `raw` is longer than new_len, but the elements over new_len are ignored + let src = unsafe { std::slice::from_raw_parts(raw, new_len) }; + dst.copy_from_slice(src); + + Ok(out) } - - Ok(out) } (_, None) => { // When the length is not known at all, collect() it first. diff --git a/src/sexp/logical.rs b/src/sexp/logical.rs index 05978cb2..76657f9e 100644 --- a/src/sexp/logical.rs +++ b/src/sexp/logical.rs @@ -1,7 +1,7 @@ use savvy_ffi::{R_NaInt, LGLSXP, LOGICAL, SET_LOGICAL_ELT, SEXP}; -use super::{impl_common_sexp_ops, impl_common_sexp_ops_owned, Sexp}; -use crate::protect; +use super::{impl_common_sexp_ops, impl_common_sexp_ops_owned, utils::assert_len, Sexp}; +use crate::protect::{self, local_protect}; /// An external SEXP of a logical vector. pub struct LogicalSexp(pub SEXP); @@ -258,27 +258,35 @@ impl OwnedLogicalSexp { // (e.g. `(0..10).filter(|x| x % 2 == 0)`), so it needs to be // truncated to the actual length at last. - let mut out = unsafe { Self::new_without_init(upper)? }; + let inner = crate::alloc_vector(LGLSXP, upper as _)?; + local_protect(inner); + let raw = unsafe { LOGICAL(inner) }; 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)?; + // and what we should not trust. + assert_len(upper, i)?; + unsafe { *(raw.add(i)) = v as _ }; last_index = i; } let new_len = last_index + 1; - if new_len != upper { - unsafe { - savvy_ffi::SETLENGTH(out.inner, new_len as _); - } - out.len = new_len; + if new_len == upper { + // If the length is the same as expected, use it as it is. + Self::new_from_raw_sexp(inner, upper) + } else { + // If the length is shorter than expected, re-allocate a new + // SEXP and copy the values into it. + let out = unsafe { Self::new_without_init(new_len)? }; + let dst = unsafe { std::slice::from_raw_parts_mut(out.raw, new_len) }; + // `raw` is longer than new_len, but the elements over new_len are ignored + let src = unsafe { std::slice::from_raw_parts(raw, new_len) }; + dst.copy_from_slice(src); + + Ok(out) } - - Ok(out) } (_, None) => { // When the length is not known at all, collect() it first. diff --git a/src/sexp/real.rs b/src/sexp/real.rs index 10767f33..e4d87e53 100644 --- a/src/sexp/real.rs +++ b/src/sexp/real.rs @@ -4,7 +4,7 @@ use savvy_ffi::{REAL, REALSXP, SEXP}; use super::utils::assert_len; use super::{impl_common_sexp_ops, impl_common_sexp_ops_owned, Sexp}; -use crate::protect; +use crate::protect::{self, local_protect}; use crate::NotAvailableValue; // for na() /// An external SEXP of a real vector. @@ -287,27 +287,35 @@ impl OwnedRealSexp { // (e.g. `(0..10).filter(|x| x % 2 == 0)`), so it needs to be // truncated to the actual length at last. - let mut out = unsafe { Self::new_without_init(upper)? }; + let inner = crate::alloc_vector(REALSXP, upper as _)?; + local_protect(inner); + let raw = unsafe { REAL(inner) }; 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)?; + // and what we should not trust. + assert_len(upper, i)?; + unsafe { *(raw.add(i)) = v }; last_index = i; } let new_len = last_index + 1; - if new_len != upper { - unsafe { - savvy_ffi::SETLENGTH(out.inner, new_len as _); - } - out.len = new_len; + if new_len == upper { + // If the length is the same as expected, use it as it is. + Self::new_from_raw_sexp(inner, upper) + } else { + // If the length is shorter than expected, re-allocate a new + // SEXP and copy the values into it. + let out = unsafe { Self::new_without_init(new_len)? }; + let dst = unsafe { std::slice::from_raw_parts_mut(out.raw, new_len) }; + // `raw` is longer than new_len, but the elements over new_len are ignored + let src = unsafe { std::slice::from_raw_parts(raw, new_len) }; + dst.copy_from_slice(src); + + Ok(out) } - - Ok(out) } (_, None) => { // When the length is not known at all, collect() it first. diff --git a/src/sexp/string.rs b/src/sexp/string.rs index 3aff3450..e1908ff5 100644 --- a/src/sexp/string.rs +++ b/src/sexp/string.rs @@ -187,28 +187,32 @@ impl OwnedStringSexp { // (e.g. `(0..10).filter(|x| x % 2 == 0)`), so it needs to be // truncated to the actual length at last. - // string doesn't have new_without_init() method - let mut out = Self::new(upper)?; + let inner = crate::alloc_vector(STRSXP, upper as _)?; + local_protect(inner); 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())?; + // and what we should not trust. + assert_len(upper, i)?; + unsafe { SET_STRING_ELT(inner, i as _, str_to_charsxp(v.as_ref())?) }; last_index = i; } let new_len = last_index + 1; - if new_len != upper { - unsafe { - savvy_ffi::SETLENGTH(out.inner, new_len as _); + if new_len == upper { + // If the length is the same as expected, use it as it is. + Self::new_from_raw_sexp(inner, upper) + } else { + // If the length is shorter than expected, re-allocate a new + // SEXP and copy the values into it. + let mut out = Self::new(new_len)?; + for i in 0..new_len { + unsafe { out.set_elt_unchecked(i, STRING_ELT(inner, i as _)) }; } - out.len = new_len; + Ok(out) } - - Ok(out) } (_, None) => { // When the length is not known at all, collect() it first. diff --git a/xtask/src/main.rs b/xtask/src/main.rs index 450e1db4..d2704950 100644 --- a/xtask/src/main.rs +++ b/xtask/src/main.rs @@ -84,7 +84,6 @@ fn show() -> Result<(), DynError> { .allowlist_function("R_IsNA") // Allocation and attributes .allowlist_function("Rf_xlength") - .allowlist_function("SETLENGTH") .allowlist_function("Rf_allocVector") .allowlist_function("Rf_install") .allowlist_function("Rf_getAttrib")