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!

Author: DavisVaughan

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_insert, store_release
 #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_insert, store_release
 #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_insert, store_release
 #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;

inst/include/cpp11/protect.hpp

@@ -245,84 +245,94 @@ void warning(const std::string& fmt, Args... args) {
 }
 #endif
 
-/// A doubly-linked list of preserved objects, allowing O(1) insertion/release of
-/// objects compared to O(N preserved) with R_PreserveObject.
-static struct {
-  SEXP insert(SEXP obj) {
-    if (obj == R_NilValue) {
-      return R_NilValue;
-    }
+namespace detail {
 
-    PROTECT(obj);
+// A doubly-linked list of preserved objects, allowing O(1) insertion/release of objects
+// compared to O(N preserved) with `R_PreserveObject()` and `R_ReleaseObject()`.
+//
+// We let R manage the memory of the list itself by calling `R_PreserveObject()` on it.
+//
+// cpp11 being a header only library makes creating a "global" preserve list a bit tricky.
+// The trick we use here is that static local variables in inline extern functions are
+// guaranteed by the standard to be unique across the whole program. Inline functions are
+// extern by default, but `static inline` functions are not, so do not change these
+// functions to `static`. If we did that, we would end up having one preserve list per
+// compilation unit instead. As it stands today, we are fairly confident that we have 1
+// preserve list per package, which seems to work nicely.
+// https://stackoverflow.com/questions/185624/what-happens-to-static-variables-in-inline-functions
+// https://stackoverflow.com/questions/51612866/global-variables-in-header-only-library
+// https://github.com/r-lib/cpp11/issues/330
+//
+// > A static local variable in an extern inline function always refers to the
+//   same object. 7.1.2/4 - C++98/C++14 (n3797)
+
+inline SEXP new_store() {
+  SEXP out = Rf_cons(R_NilValue, Rf_cons(R_NilValue, R_NilValue));
+  R_PreserveObject(out);
+  return out;
+}
 
-    static SEXP list = get_preserve_list();
+inline SEXP store() {
+  // Note the `static` local variable in the inline extern function here! Guarantees we
+  // have 1 unique preserve list across all compilation units in the package.
+  static SEXP out = new_store();
+  return out;
+}
 
-    // Get references to the head of the precious list and the next element
-    // after the head
-    SEXP head = list;
-    SEXP next = CDR(list);
+inline SEXP store_insert(SEXP x) {
+  if (x == R_NilValue) {
+    return R_NilValue;
+  }
 
-    // Add a new cell that points to the current head + next.
-    SEXP cell = PROTECT(Rf_cons(head, next));
-    SET_TAG(cell, obj);
+  PROTECT(x);
 
-    // Update the head + next to point at the newly-created cell,
-    // effectively inserting that cell between the current head + next.
-    SETCDR(head, cell);
-    SETCAR(next, cell);
+  SEXP list = store();
 
-    UNPROTECT(2);
+  // Get references to the head of the preserve list and the next element
+  // after the head
+  SEXP head = list;
+  SEXP next = CDR(list);
 
-    return cell;
-  }
+  // Add a new cell that points to the current head + next.
+  SEXP cell = PROTECT(Rf_cons(head, next));
+  SET_TAG(cell, x);
 
-  void print() {
-    static SEXP list = get_preserve_list();
-    for (SEXP cell = list; cell != R_NilValue; cell = CDR(cell)) {
-      REprintf("%p CAR: %p CDR: %p TAG: %p\n", reinterpret_cast<void*>(cell),
-               reinterpret_cast<void*>(CAR(cell)), reinterpret_cast<void*>(CDR(cell)),
-               reinterpret_cast<void*>(TAG(cell)));
-    }
-    REprintf("---\n");
-  }
+  // Update the head + next to point at the newly-created cell,
+  // effectively inserting that cell between the current head + next.
+  SETCDR(head, cell);
+  SETCAR(next, cell);
 
-  void release(SEXP cell) {
-    if (cell == R_NilValue) {
-      return;
-    }
+  UNPROTECT(2);
 
-    // Get a reference to the cells before and after the token.
-    SEXP lhs = CAR(cell);
-    SEXP rhs = CDR(cell);
+  return cell;
+}
 
-    // Remove the cell from the precious list -- effectively, we do this
-    // by updating the 'lhs' and 'rhs' references to point at each-other,
-    // effectively removing any references to the cell in the pairlist.
-    SETCDR(lhs, rhs);
-    SETCAR(rhs, lhs);
+inline void store_release(SEXP cell) {
+  if (cell == R_NilValue) {
+    return;
   }
 
- private:
-  // Each compilation unit purposefully gets its own preserve list.
-  // This avoids issues with sharing preserve list state across compilation units
-  // and across packages, which has historically caused many issues (#330).
-  static SEXP get_preserve_list() {
-    static SEXP out = init_preserve_list();
-    return out;
-  }
+  // Get a reference to the cells before and after the token.
+  SEXP lhs = CAR(cell);
+  SEXP rhs = CDR(cell);
 
-  static SEXP init_preserve_list() {
-    // Initialize the list exactly once per compilation unit,
-    // and let R manage its memory
-    SEXP out = new_preserve_list();
-    R_PreserveObject(out);
-    return out;
-  }
+  // Remove the cell from the preserve list -- effectively, we do this
+  // by updating the 'lhs' and 'rhs' references to point at each-other,
+  // effectively removing any references to the cell in the pairlist.
+  SETCDR(lhs, rhs);
+  SETCAR(rhs, lhs);
+}
 
-  static SEXP new_preserve_list() {
-    return Rf_cons(R_NilValue, Rf_cons(R_NilValue, R_NilValue));
+inline void store_print() {
+  SEXP list = store();
+  for (SEXP cell = list; cell != R_NilValue; cell = CDR(cell)) {
+    REprintf("%p CAR: %p CDR: %p TAG: %p\n", reinterpret_cast<void*>(cell),
+             reinterpret_cast<void*>(CAR(cell)), reinterpret_cast<void*>(CDR(cell)),
+             reinterpret_cast<void*>(TAG(cell)));
   }
+  REprintf("---\n");
+}
 
-} preserved;
+}  // namespace detail
 
 }  // namespace cpp11

inst/include/cpp11/r_bool.hpp

@@ -7,7 +7,7 @@
 #include "R_ext/Boolean.h"    // for Rboolean
 #include "cpp11/R.hpp"        // for SEXP, SEXPREC, ...
 #include "cpp11/as.hpp"       // for as_sexp
-#include "cpp11/protect.hpp"  // for unwind_protect, preserved
+#include "cpp11/protect.hpp"  // for unwind_protect
 #include "cpp11/r_vector.hpp"
 #include "cpp11/sexp.hpp"  // for sexp