Simplify preserve list approach (#364)

* Simplify preserve list approach

And finally provide some reasoning backed by the standard about how the preserve list is unique across the compilation units in the package!

* Formatting

* NEWS bullet

* NEWS bullet tweak

* Create internal `store` namespace

Author: DavisVaughan

NEWS.md

@@ -1,5 +1,17 @@
 # cpp11 (development version)
 
+* The approach for the protection list managed by cpp11 has been tweaked
+  slightly. In 0.4.6, we changed to an approach that creates one protection list
+  per compilation unit, but we now believe we've found an approach that is
+  guaranteed by the C++ standard to create one protection list per package,
+  which makes slightly more sense and still has all the benefits of the reduced
+  maintanence burden mentioned in the 0.4.6 news bullet (#364).
+
+  A side effect of this new approach is that the `preserved` object exposed
+  through `protect.hpp` no longer exists. We don't believe that anyone was using
+  this. This also means you should no longer see "unused variable" warnings
+  about `preserved` (#249).
+
 * Dropped support for gcc 4.8, mainly an issue for extremely old CentOS 7
   systems which used that as their default compiler. As of June 2024, CentOS 7
   is past its Vendor end of support date and therefore also out of scope for

cpp11test/src/protect.cpp

@@ -18,8 +18,8 @@
 
 [[cpp11::register]] void protect_one_cpp11_(SEXP x, int n) {
   for (R_xlen_t i = 0; i < n; ++i) {
-    SEXP p = cpp11::preserved.insert(x);
-    cpp11::preserved.release(p);
+    SEXP p = cpp11::detail::store::insert(x);
+    cpp11::detail::store::release(p);
   }
 }
 
@@ -32,18 +32,6 @@
 
 // The internal protections here are actually uneeded, but it is a useful way to benchmark
 // them
-//
-// clang-format off
-#ifdef __clang__
-# pragma clang diagnostic push
-# pragma clang diagnostic ignored "-Wunused-variable"
-#endif
-
-#ifdef __GNUC__
-# pragma GCC diagnostic push
-# pragma GCC diagnostic ignored "-Wunused-variable"
-#endif
-// clang-format on
 
 [[cpp11::register]] void protect_many_(int n) {
 #ifdef CPP11_BENCH
@@ -63,12 +51,12 @@
 [[cpp11::register]] void protect_many_cpp11_(int n) {
   std::vector<SEXP> res;
   for (R_xlen_t i = 0; i < n; ++i) {
-    res.push_back(cpp11::preserved.insert(Rf_ScalarInteger(n)));
+    res.push_back(cpp11::detail::store::insert(Rf_ScalarInteger(n)));
   }
 
   for (R_xlen_t i = n - 1; i >= 0; --i) {
     SEXP x = res[i];
-    cpp11::preserved.release(x);
+    cpp11::detail::store::release(x);
     res.pop_back();
   }
 }

cpp11test/src/test-protect.cpp

@@ -1,16 +1,3 @@
-// Avoid unused variable warnings regarding `cpp11::preserved`
-// clang-format off
-#ifdef __clang__
-# pragma clang diagnostic push
-# pragma clang diagnostic ignored "-Wunused-variable"
-#endif
-
-#ifdef __GNUC__
-# pragma GCC diagnostic push
-# pragma GCC diagnostic ignored "-Wunused-variable"
-#endif
-// clang-format on
-
 #define CPP11_USE_FMT
 #include "cpp11/protect.hpp"
 #include "testthat.h"

inst/include/cpp11/doubles.hpp

@@ -84,7 +84,7 @@ template <>
 inline r_vector<double>::r_vector(std::initializer_list<named_arg> il)
     : cpp11::r_vector<double>(safe[Rf_allocVector](REALSXP, il.size())),
       capacity_(il.size()) {
-  protect_ = preserved.insert(data_);
+  protect_ = detail::store::insert(data_);
   int n_protected = 0;
 
   try {
@@ -100,7 +100,7 @@ inline r_vector<double>::r_vector(std::initializer_list<named_arg> il)
       UNPROTECT(n_protected);
     });
   } catch (const unwind_exception& e) {
-    preserved.release(protect_);
+    detail::store::release(protect_);
     UNPROTECT(n_protected);
     throw e;
   }
@@ -111,8 +111,8 @@ inline void r_vector<double>::reserve(R_xlen_t new_capacity) {
   data_ = data_ == R_NilValue ? safe[Rf_allocVector](REALSXP, new_capacity)
                               : safe[Rf_xlengthgets](data_, new_capacity);
   SEXP old_protect = protect_;
-  protect_ = preserved.insert(data_);
-  preserved.release(old_protect);
+  protect_ = detail::store::insert(data_);
+  detail::store::release(old_protect);
 
   data_p_ = REAL(data_);
   capacity_ = new_capacity;

inst/include/cpp11/integers.hpp

@@ -9,7 +9,7 @@
 #include "cpp11/as.hpp"               // for as_sexp
 #include "cpp11/attribute_proxy.hpp"  // for attribute_proxy
 #include "cpp11/named_arg.hpp"        // for named_arg
-#include "cpp11/protect.hpp"          // for preserved
+#include "cpp11/protect.hpp"          // for store
 #include "cpp11/r_vector.hpp"         // for r_vector, r_vector<>::proxy
 #include "cpp11/sexp.hpp"             // for sexp
 
@@ -87,10 +87,10 @@ inline void r_vector<int>::reserve(R_xlen_t new_capacity) {
   SEXP old_protect = protect_;
 
   // Protect the new data
-  protect_ = preserved.insert(data_);
+  protect_ = detail::store::insert(data_);
 
   // Release the old protection;
-  preserved.release(old_protect);
+  detail::store::release(old_protect);
 
   data_p_ = INTEGER(data_);
   capacity_ = new_capacity;
@@ -100,7 +100,7 @@ template <>
 inline r_vector<int>::r_vector(std::initializer_list<named_arg> il)
     : cpp11::r_vector<int>(safe[Rf_allocVector](INTSXP, il.size())),
       capacity_(il.size()) {
-  protect_ = preserved.insert(data_);
+  protect_ = detail::store::insert(data_);
   int n_protected = 0;
 
   try {
@@ -116,7 +116,7 @@ inline r_vector<int>::r_vector(std::initializer_list<named_arg> il)
       UNPROTECT(n_protected);
     });
   } catch (const unwind_exception& e) {
-    preserved.release(protect_);
+    detail::store::release(protect_);
     UNPROTECT(n_protected);
     throw e;
   }

inst/include/cpp11/list.hpp

@@ -5,7 +5,7 @@
 #include "cpp11/R.hpp"                // for SEXP, SEXPREC, SET_VECTOR_ELT
 #include "cpp11/attribute_proxy.hpp"  // for attribute_proxy
 #include "cpp11/named_arg.hpp"        // for named_arg
-#include "cpp11/protect.hpp"          // for preserved
+#include "cpp11/protect.hpp"          // for store
 #include "cpp11/r_string.hpp"         // for r_string
 #include "cpp11/r_vector.hpp"         // for r_vector, r_vector<>::proxy
 #include "cpp11/sexp.hpp"             // for sexp
@@ -78,7 +78,7 @@ template <>
 inline r_vector<SEXP>::r_vector(std::initializer_list<SEXP> il)
     : cpp11::r_vector<SEXP>(safe[Rf_allocVector](VECSXP, il.size())),
       capacity_(il.size()) {
-  protect_ = preserved.insert(data_);
+  protect_ = detail::store::insert(data_);
   auto it = il.begin();
   for (R_xlen_t i = 0; i < capacity_; ++i, ++it) {
     SET_VECTOR_ELT(data_, i, *it);
@@ -89,7 +89,7 @@ template <>
 inline r_vector<SEXP>::r_vector(std::initializer_list<named_arg> il)
     : cpp11::r_vector<SEXP>(safe[Rf_allocVector](VECSXP, il.size())),
       capacity_(il.size()) {
-  protect_ = preserved.insert(data_);
+  protect_ = detail::store::insert(data_);
   int n_protected = 0;
 
   try {
@@ -105,7 +105,7 @@ inline r_vector<SEXP>::r_vector(std::initializer_list<named_arg> il)
       UNPROTECT(n_protected);
     });
   } catch (const unwind_exception& e) {
-    preserved.release(protect_);
+    detail::store::release(protect_);
     UNPROTECT(n_protected);
     throw e;
   }
@@ -117,8 +117,8 @@ inline void r_vector<SEXP>::reserve(R_xlen_t new_capacity) {
                               : safe[Rf_xlengthgets](data_, new_capacity);
 
   SEXP old_protect = protect_;
-  protect_ = preserved.insert(data_);
-  preserved.release(old_protect);
+  protect_ = detail::store::insert(data_);
+  detail::store::release(old_protect);
 
   capacity_ = new_capacity;
 }

inst/include/cpp11/logicals.hpp

@@ -7,7 +7,7 @@
 #include "cpp11/R.hpp"                // for SEXP, SEXPREC, Rf_all...
 #include "cpp11/attribute_proxy.hpp"  // for attribute_proxy
 #include "cpp11/named_arg.hpp"        // for named_arg
-#include "cpp11/protect.hpp"          // for preserved
+#include "cpp11/protect.hpp"          // for store
 #include "cpp11/r_bool.hpp"           // for r_bool
 #include "cpp11/r_vector.hpp"         // for r_vector, r_vector<>::proxy
 #include "cpp11/sexp.hpp"             // for sexp
@@ -80,7 +80,7 @@ inline bool operator==(const r_vector<r_bool>::proxy& lhs, r_bool rhs) {
 template <>
 inline r_vector<r_bool>::r_vector(std::initializer_list<r_bool> il)
     : cpp11::r_vector<r_bool>(Rf_allocVector(LGLSXP, il.size())), capacity_(il.size()) {
-  protect_ = preserved.insert(data_);
+  protect_ = detail::store::insert(data_);
   auto it = il.begin();
   for (R_xlen_t i = 0; i < capacity_; ++i, ++it) {
     SET_LOGICAL_ELT(data_, i, *it);
@@ -91,7 +91,7 @@ template <>
 inline r_vector<r_bool>::r_vector(std::initializer_list<named_arg> il)
     : cpp11::r_vector<r_bool>(safe[Rf_allocVector](LGLSXP, il.size())),
       capacity_(il.size()) {
-  protect_ = preserved.insert(data_);
+  protect_ = detail::store::insert(data_);
   int n_protected = 0;
 
   try {
@@ -107,7 +107,7 @@ inline r_vector<r_bool>::r_vector(std::initializer_list<named_arg> il)
       UNPROTECT(n_protected);
     });
   } catch (const unwind_exception& e) {
-    preserved.release(protect_);
+    detail::store::release(protect_);
     UNPROTECT(n_protected);
     throw e;
   }
@@ -118,9 +118,9 @@ inline void r_vector<r_bool>::reserve(R_xlen_t new_capacity) {
   data_ = data_ == R_NilValue ? safe[Rf_allocVector](LGLSXP, new_capacity)
                               : safe[Rf_xlengthgets](data_, new_capacity);
   SEXP old_protect = protect_;
-  protect_ = preserved.insert(data_);
+  protect_ = detail::store::insert(data_);
 
-  preserved.release(old_protect);
+  detail::store::release(old_protect);
 
   data_p_ = LOGICAL(data_);
   capacity_ = new_capacity;