Skip to content

Commit 5b72c9b

Browse files
yutannihilationyutannihilation
authoredApr 26, 2024··
Do not use SETLENGTH (#214)
·
v0.8.6v0.6.1
1 parent bf82415 commit 5b72c9b

File tree

9 files changed

+102
-63
lines changed

9 files changed

+102
-63
lines changed
 

‎.github/workflows/check-non-api.yaml

+3-1
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,7 @@ jobs:
2626
2727
REGEX=$(Rscript tmp.R)
2828
29-
NON_API=$(grep -R -E "${REGEX}" ./savvy-ffi/src/)
29+
NON_API=$(grep -R -E "${REGEX}" ./savvy-ffi/src/ || true)
3030
3131
echo "Check result:"
3232
echo
@@ -36,4 +36,6 @@ jobs:
3636
if [ -n "${NON_API}" ]; then
3737
echo 'Found what they call "non-API"!'
3838
exit 1
39+
else
40+
echo "OK!"
3941
fi

‎CHANGELOG.md

+4
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,10 @@
33
<!-- next-header -->
44
## [Unreleased] (ReleaseDate)
55

6+
### Minor improvements
7+
8+
* Now savvy no longer uses `SETLENGTH`, which is a so-called "non-API" thing.
9+
610
## [v0.6.0] (2024-04-20)
711

812
### Breaking changes

‎savvy-ffi/src/lib.rs

-1
Original file line numberDiff line numberDiff line change
@@ -69,7 +69,6 @@ extern "C" {
6969
// Allocation and attributes
7070
extern "C" {
7171
pub fn Rf_xlength(arg1: SEXP) -> R_xlen_t;
72-
pub fn SETLENGTH(x: SEXP, v: R_xlen_t);
7372
pub fn Rf_allocVector(arg1: SEXPTYPE, arg2: R_xlen_t) -> SEXP;
7473
pub fn Rf_install(arg1: *const ::std::os::raw::c_char) -> SEXP;
7574
pub fn Rf_getAttrib(arg1: SEXP, arg2: SEXP) -> SEXP;

‎src/sexp/complex.rs

+19-12
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,7 @@ use savvy_ffi::CPLXSXP;
55
use savvy_ffi::{COMPLEX, SEXP};
66

77
use super::{impl_common_sexp_ops, impl_common_sexp_ops_owned, utils::assert_len, Sexp};
8-
use crate::protect;
8+
use crate::protect::{self, local_protect};
99
use crate::NotAvailableValue; // for na()
1010

1111
/// An external SEXP of a complex vector.
@@ -198,27 +198,34 @@ impl OwnedComplexSexp {
198198
// (e.g. `(0..10).filter(|x| x % 2 == 0)`), so it needs to be
199199
// truncated to the actual length at last.
200200

201-
let mut out = unsafe { Self::new_without_init(upper)? };
201+
let inner = crate::alloc_vector(CPLXSXP, upper as _)?;
202+
local_protect(inner);
203+
let raw = unsafe { COMPLEX(inner) };
202204

203205
let mut last_index = 0;
204206
for (i, v) in iter.enumerate() {
205207
// The upper bound of size_hint() is just for optimization
206-
// and what we should not trust. So, we should't use
207-
// `set_elt_unchecked()` here.
208-
out.set_elt(i, v)?;
208+
// and what we should not trust.
209+
assert_len(upper, i)?;
210+
unsafe { *(raw.add(i)) = v };
209211

210212
last_index = i;
211213
}
212214

213215
let new_len = last_index + 1;
214-
if new_len != upper {
215-
unsafe {
216-
savvy_ffi::SETLENGTH(out.inner, new_len as _);
217-
}
218-
out.len = new_len;
216+
if new_len == upper {
217+
// If the length is the same as expected, use it as it is.
218+
Self::new_from_raw_sexp(inner, upper)
219+
} else {
220+
// If the length is shorter than expected, re-allocate a new
221+
// SEXP and copy the values into it.
222+
let mut out = unsafe { Self::new_without_init(new_len)? };
223+
// `raw` is longer than new_len, but the elements over new_len are ignored
224+
let raw_slice = unsafe { std::slice::from_raw_parts(raw, new_len) };
225+
out.as_mut_slice().copy_from_slice(raw_slice);
226+
227+
Ok(out)
219228
}
220-
221-
Ok(out)
222229
}
223230
(_, None) => {
224231
// When the length is not known at all, collect() it first.

‎src/sexp/integer.rs

+20-12
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,7 @@ use savvy_ffi::{INTEGER, INTSXP, SEXP};
44

55
use super::utils::assert_len;
66
use super::{impl_common_sexp_ops, impl_common_sexp_ops_owned, Sexp};
7-
use crate::protect;
7+
use crate::protect::{self, local_protect};
88
use crate::NotAvailableValue; // for na()
99

1010
/// An external SEXP of an integer vector.
@@ -284,27 +284,35 @@ impl OwnedIntegerSexp {
284284
// (e.g. `(0..10).filter(|x| x % 2 == 0)`), so it needs to be
285285
// truncated to the actual length at last.
286286

287-
let mut out = unsafe { Self::new_without_init(upper)? };
287+
let inner = crate::alloc_vector(INTSXP, upper as _)?;
288+
local_protect(inner);
289+
let raw = unsafe { INTEGER(inner) };
288290

289291
let mut last_index = 0;
290292
for (i, v) in iter.enumerate() {
291293
// The upper bound of size_hint() is just for optimization
292-
// and what we should not trust. So, we should't use
293-
// `set_elt_unchecked()` here.
294-
out.set_elt(i, v)?;
294+
// and what we should not trust.
295+
assert_len(upper, i)?;
296+
unsafe { *(raw.add(i)) = v };
295297

296298
last_index = i;
297299
}
298300

299301
let new_len = last_index + 1;
300-
if new_len != upper {
301-
unsafe {
302-
savvy_ffi::SETLENGTH(out.inner, new_len as _);
303-
}
304-
out.len = new_len;
302+
if new_len == upper {
303+
// If the length is the same as expected, use it as it is.
304+
Self::new_from_raw_sexp(inner, upper)
305+
} else {
306+
// If the length is shorter than expected, re-allocate a new
307+
// SEXP and copy the values into it.
308+
let out = unsafe { Self::new_without_init(new_len)? };
309+
let dst = unsafe { std::slice::from_raw_parts_mut(out.raw, new_len) };
310+
// `raw` is longer than new_len, but the elements over new_len are ignored
311+
let src = unsafe { std::slice::from_raw_parts(raw, new_len) };
312+
dst.copy_from_slice(src);
313+
314+
Ok(out)
305315
}
306-
307-
Ok(out)
308316
}
309317
(_, None) => {
310318
// When the length is not known at all, collect() it first.

‎src/sexp/logical.rs

+21-13
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
use savvy_ffi::{R_NaInt, LGLSXP, LOGICAL, SET_LOGICAL_ELT, SEXP};
22

3-
use super::{impl_common_sexp_ops, impl_common_sexp_ops_owned, Sexp};
4-
use crate::protect;
3+
use super::{impl_common_sexp_ops, impl_common_sexp_ops_owned, utils::assert_len, Sexp};
4+
use crate::protect::{self, local_protect};
55

66
/// An external SEXP of a logical vector.
77
pub struct LogicalSexp(pub SEXP);
@@ -258,27 +258,35 @@ impl OwnedLogicalSexp {
258258
// (e.g. `(0..10).filter(|x| x % 2 == 0)`), so it needs to be
259259
// truncated to the actual length at last.
260260

261-
let mut out = unsafe { Self::new_without_init(upper)? };
261+
let inner = crate::alloc_vector(LGLSXP, upper as _)?;
262+
local_protect(inner);
263+
let raw = unsafe { LOGICAL(inner) };
262264

263265
let mut last_index = 0;
264266
for (i, v) in iter.enumerate() {
265267
// The upper bound of size_hint() is just for optimization
266-
// and what we should not trust. So, we should't use
267-
// `set_elt_unchecked()` here.
268-
out.set_elt(i, v)?;
268+
// and what we should not trust.
269+
assert_len(upper, i)?;
270+
unsafe { *(raw.add(i)) = v as _ };
269271

270272
last_index = i;
271273
}
272274

273275
let new_len = last_index + 1;
274-
if new_len != upper {
275-
unsafe {
276-
savvy_ffi::SETLENGTH(out.inner, new_len as _);
277-
}
278-
out.len = new_len;
276+
if new_len == upper {
277+
// If the length is the same as expected, use it as it is.
278+
Self::new_from_raw_sexp(inner, upper)
279+
} else {
280+
// If the length is shorter than expected, re-allocate a new
281+
// SEXP and copy the values into it.
282+
let out = unsafe { Self::new_without_init(new_len)? };
283+
let dst = unsafe { std::slice::from_raw_parts_mut(out.raw, new_len) };
284+
// `raw` is longer than new_len, but the elements over new_len are ignored
285+
let src = unsafe { std::slice::from_raw_parts(raw, new_len) };
286+
dst.copy_from_slice(src);
287+
288+
Ok(out)
279289
}
280-
281-
Ok(out)
282290
}
283291
(_, None) => {
284292
// When the length is not known at all, collect() it first.

‎src/sexp/real.rs

+20-12
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,7 @@ use savvy_ffi::{REAL, REALSXP, SEXP};
44

55
use super::utils::assert_len;
66
use super::{impl_common_sexp_ops, impl_common_sexp_ops_owned, Sexp};
7-
use crate::protect;
7+
use crate::protect::{self, local_protect};
88
use crate::NotAvailableValue; // for na()
99

1010
/// An external SEXP of a real vector.
@@ -287,27 +287,35 @@ impl OwnedRealSexp {
287287
// (e.g. `(0..10).filter(|x| x % 2 == 0)`), so it needs to be
288288
// truncated to the actual length at last.
289289

290-
let mut out = unsafe { Self::new_without_init(upper)? };
290+
let inner = crate::alloc_vector(REALSXP, upper as _)?;
291+
local_protect(inner);
292+
let raw = unsafe { REAL(inner) };
291293

292294
let mut last_index = 0;
293295
for (i, v) in iter.enumerate() {
294296
// The upper bound of size_hint() is just for optimization
295-
// and what we should not trust. So, we should't use
296-
// `set_elt_unchecked()` here.
297-
out.set_elt(i, v)?;
297+
// and what we should not trust.
298+
assert_len(upper, i)?;
299+
unsafe { *(raw.add(i)) = v };
298300

299301
last_index = i;
300302
}
301303

302304
let new_len = last_index + 1;
303-
if new_len != upper {
304-
unsafe {
305-
savvy_ffi::SETLENGTH(out.inner, new_len as _);
306-
}
307-
out.len = new_len;
305+
if new_len == upper {
306+
// If the length is the same as expected, use it as it is.
307+
Self::new_from_raw_sexp(inner, upper)
308+
} else {
309+
// If the length is shorter than expected, re-allocate a new
310+
// SEXP and copy the values into it.
311+
let out = unsafe { Self::new_without_init(new_len)? };
312+
let dst = unsafe { std::slice::from_raw_parts_mut(out.raw, new_len) };
313+
// `raw` is longer than new_len, but the elements over new_len are ignored
314+
let src = unsafe { std::slice::from_raw_parts(raw, new_len) };
315+
dst.copy_from_slice(src);
316+
317+
Ok(out)
308318
}
309-
310-
Ok(out)
311319
}
312320
(_, None) => {
313321
// When the length is not known at all, collect() it first.

‎src/sexp/string.rs

+15-11
Original file line numberDiff line numberDiff line change
@@ -187,28 +187,32 @@ impl OwnedStringSexp {
187187
// (e.g. `(0..10).filter(|x| x % 2 == 0)`), so it needs to be
188188
// truncated to the actual length at last.
189189

190-
// string doesn't have new_without_init() method
191-
let mut out = Self::new(upper)?;
190+
let inner = crate::alloc_vector(STRSXP, upper as _)?;
191+
local_protect(inner);
192192

193193
let mut last_index = 0;
194194
for (i, v) in iter.enumerate() {
195195
// The upper bound of size_hint() is just for optimization
196-
// and what we should not trust. So, we should't use
197-
// `set_elt_unchecked()` here.
198-
out.set_elt(i, v.as_ref())?;
196+
// and what we should not trust.
197+
assert_len(upper, i)?;
198+
unsafe { SET_STRING_ELT(inner, i as _, str_to_charsxp(v.as_ref())?) };
199199

200200
last_index = i;
201201
}
202202

203203
let new_len = last_index + 1;
204-
if new_len != upper {
205-
unsafe {
206-
savvy_ffi::SETLENGTH(out.inner, new_len as _);
204+
if new_len == upper {
205+
// If the length is the same as expected, use it as it is.
206+
Self::new_from_raw_sexp(inner, upper)
207+
} else {
208+
// If the length is shorter than expected, re-allocate a new
209+
// SEXP and copy the values into it.
210+
let mut out = Self::new(new_len)?;
211+
for i in 0..new_len {
212+
unsafe { out.set_elt_unchecked(i, STRING_ELT(inner, i as _)) };
207213
}
208-
out.len = new_len;
214+
Ok(out)
209215
}
210-
211-
Ok(out)
212216
}
213217
(_, None) => {
214218
// When the length is not known at all, collect() it first.

‎xtask/src/main.rs

-1
Original file line numberDiff line numberDiff line change
@@ -84,7 +84,6 @@ fn show() -> Result<(), DynError> {
8484
.allowlist_function("R_IsNA")
8585
// Allocation and attributes
8686
.allowlist_function("Rf_xlength")
87-
.allowlist_function("SETLENGTH")
8887
.allowlist_function("Rf_allocVector")
8988
.allowlist_function("Rf_install")
9089
.allowlist_function("Rf_getAttrib")

0 commit comments

Comments
 (0)
Please sign in to comment.