Remove non-API calls to R: SETLENGTH, SET_TRUELENGTH Fix #355

[nx10]

Seems like R/CRAN does not want these used publicly anymore starting with 4.5.*

See details in #355

[nx10]

For reference gcc 4.8 CI failing is unrelated: #356

[chainsawriot]

@nx10 If you do it this way, you also have to modify this as well

cpp11/inst/include/cpp11/r_vector.hpp

Lines 910 to 926 in 51f4cd5

	inline r_vector<T>::operator SEXP() const {
	auto* p = const_cast<r_vector<T>*>(this);
	if (data_ == R_NilValue) {
	p->resize(0);
	return data_;
	}
	if (length_ < capacity_) {
	p->data_ = truncate(p->data_, length_, capacity_);
	SEXP nms = names();
	auto nms_size = Rf_xlength(nms);
	if ((nms_size > 0) && (length_ < nms_size)) {
	nms = truncate(nms, length_, capacity_);
	names() = nms;
	}
	}
	return data_;
	}

Or else it will trigger rchk issues

ref ropensci/readODS#190

[mtmorgan]

I see this issue in the CRAN checks for rjsoncons too.

Reverting to Rf_lengthgets() as in the PR seems like a simple solution even though it now (might?) cause a copy; should it be Rf_xlengthgets(), since elsewhere the code uses Rf_xlength()

diff --git a/inst/include/cpp11/r_vector.hpp b/inst/include/cpp11/r_vector.hpp
index 4831c2f..eee63f7 100644
--- a/inst/include/cpp11/r_vector.hpp
+++ b/inst/include/cpp11/r_vector.hpp
@@ -895,14 +895,8 @@ inline void r_vector<T>::clear() {
   length_ = 0;
 }
 
-inline SEXP truncate(SEXP x, R_xlen_t length, R_xlen_t capacity) {
-#if R_VERSION >= R_Version(3, 4, 0)
-  SETLENGTH(x, length);
-  SET_TRUELENGTH(x, capacity);
-  SET_GROWABLE_BIT(x);
-#else
-  x = safe[Rf_lengthgets](x, length);
-#endif
+inline SEXP truncate(SEXP x, R_xlen_t length) {
+  x = safe[Rf_xlengthgets](x, length);
   return x;
 }
 
@@ -914,11 +908,11 @@ inline r_vector<T>::operator SEXP() const {
     return data_;
   }
   if (length_ < capacity_) {
-    p->data_ = truncate(p->data_, length_, capacity_);
+    p->data_ = truncate(p->data_, length_);
     SEXP nms = names();
     auto nms_size = Rf_xlength(nms);
     if ((nms_size > 0) && (length_ < nms_size)) {
-      nms = truncate(nms, length_, capacity_);
+      nms = truncate(nms, length_);
       names() = nms;
     }
   }

@chainsawriot seems to suggest PROTECT()ing names(), which I guess satisfies rchk but doesn't seem necessary, names() is accessing an attribute of data_, and data_ and its attributes are already preserved. On the other hand it looks like @jimhester originally introduced PROTECT() in a number of places to satisfy rchk even though probably not necessary.

Rf_xlengthgets() and truncate() "return a freshly allocated object" (Writing R Extensions 5.9.2). Should the return value be PROTECT()ed? My C++ isn't strong enough to fully understand what the assignment p->data_ = truncate(...) does; if it's analogous to p->data_ = sexp(truncate(...)) then no protection is necessary because sexp() immediately calls preserved.insert() which immediately protects it's argument. I am also not confident enough about what names() = nms does, and whether nms is immediately protected....

[jimhester]

There will likely need to be benchmarking done to see how badly this effects performance. It was done this way to avoid an extra copy of the data when truncating. But additionally unwind protection used in safe also has some overhead when called in hot loops.

As this function gets called when coercing cpp11 vectors to SEXP it is a pretty hot area of the code.

[DavisVaughan]

We will be looking at this soon to get a patch release of cpp11 out. It seems that the R-devel non-API tweaks have slowed down to the point that we should be able to send a release in without much fear of something else additionally getting marked non-API later on.

We are going to lose access to SETLENGTH() and friends, so we will benchmark and if it turns out to be too painful we will try and think of something else.

In the meantime, if you have a package that uses cpp11 you can send in a release of that package even with the NOTE. CRAN is aware of it and will approve your package.