Implement copy assignment for proxy (#391)

Author: DavisVaughan

NEWS.md

@@ -1,5 +1,9 @@
 # cpp11 (development version)
 
+* `cpp11::writable::r_vector<T>::proxy` now implements copy assignment.
+  Practically this means that `x[i] = y[i]` now works when both `x` and `y`
+  are writable vectors (#300, #339).
+
 * Implicit conversion from `sexp` to `bool`, `size_t`, and `double` has been
   marked as deprecated and will be removed in the next version of cpp11. The 3
   packages that were using this have been notified and sent PRs. The recommended

cpp11test/src/test-r_vector.cpp

@@ -1,6 +1,7 @@
 #include "cpp11/integers.hpp"
 #include "cpp11/list.hpp"
 #include "cpp11/protect.hpp"
+#include "cpp11/strings.hpp"
 
 #include <testthat.h>
 
@@ -72,12 +73,8 @@ context("r_vector-capabilities-C++") {
     expect_true(std::is_trivially_destructible<integers::proxy>::value);
     expect_true(std::is_copy_constructible<integers::proxy>::value);
     expect_true(std::is_move_constructible<integers::proxy>::value);
-
-    // Should these be true? Does it affect anything in practice?
-    expect_false(std::is_copy_assignable<integers::proxy>::value);
-    expect_false(std::is_trivially_copy_assignable<integers::proxy>::value);
-    expect_false(std::is_move_assignable<integers::proxy>::value);
-    expect_false(std::is_trivially_move_assignable<integers::proxy>::value);
+    expect_true(std::is_copy_assignable<integers::proxy>::value);
+    expect_true(std::is_move_assignable<integers::proxy>::value);
   }
 }
 #endif
@@ -475,6 +472,66 @@ context("r_vector-C++") {
     UNPROTECT(2);
   }
 
+  test_that("`proxy` is copy assignable (integers) (#300, #339)") {
+    cpp11::writable::integers foo = {1, 2, 3, 4, 5};
+    cpp11::writable::integers bar = {6, 7, 8, 9, 10};
+
+    // Using rvalue temporaries (i.e. move assignable, but using copy assignment operator)
+    for (R_xlen_t i = 0; i < foo.size(); ++i) {
+      bar[i] = foo[i];
+    }
+
+    // Using lvalues (i.e. copy assignable)
+    cpp11::writable::integers::proxy x = foo[0];
+    bar[4] = x;
+
+    expect_true(bar[0] == 1);
+    expect_true(bar[1] == 2);
+    expect_true(bar[2] == 3);
+    expect_true(bar[3] == 4);
+    expect_true(bar[4] == 1);
+  }
+
+  test_that("`proxy` is copy assignable (list) (#300, #339)") {
+    SEXP a = PROTECT(Rf_allocVector(INTSXP, 1));
+    SEXP b = PROTECT(Rf_allocVector(REALSXP, 2));
+
+    cpp11::writable::list x({a, b});
+    cpp11::writable::list y(2);
+
+    // Using rvalue temporaries (i.e. move assignable, but using copy assignment operator)
+    y[0] = x[0];
+
+    // Using lvalues (i.e. copy assignable)
+    cpp11::writable::list::proxy elt = x[1];
+    y[1] = elt;
+
+    expect_true(y[0] == a);
+    expect_true(y[1] == b);
+
+    UNPROTECT(2);
+  }
+
+  test_that("`proxy` is copy assignable (strings) (#300, #339)") {
+    SEXP a = PROTECT(Rf_mkCharCE("a", CE_UTF8));
+    SEXP b = PROTECT(Rf_mkCharCE("b", CE_UTF8));
+
+    cpp11::writable::strings x({a, b});
+    cpp11::writable::strings y(2);
+
+    // Using rvalue temporaries (i.e. move assignable, but using copy assignment operator)
+    y[0] = x[0];
+
+    // Using lvalues (i.e. copy assignable)
+    cpp11::writable::strings::proxy elt = x[1];
+    y[1] = elt;
+
+    expect_true(y[0] == a);
+    expect_true(y[1] == b);
+
+    UNPROTECT(2);
+  }
+
   test_that("std::max_element works on read only vectors") {
     SEXP foo_sexp = PROTECT(Rf_allocVector(INTSXP, 5));
     SET_INTEGER_ELT(foo_sexp, 0, 1);

inst/include/cpp11/r_vector.hpp

@@ -272,6 +272,8 @@ class r_vector : public cpp11::r_vector<T> {
    public:
     proxy(SEXP data, const R_xlen_t index, underlying_type* const p, bool is_altrep);
 
+    proxy& operator=(const proxy& rhs);
+
     proxy& operator=(const T& rhs);
     proxy& operator+=(const T& rhs);
     proxy& operator-=(const T& rhs);
@@ -284,6 +286,10 @@ class r_vector : public cpp11::r_vector<T> {
     void operator--();
 
     operator T() const;
+
+   private:
+    underlying_type get() const;
+    void set(underlying_type x);
   };
 
   class iterator : public cpp11::r_vector<T>::const_iterator {
@@ -1176,16 +1182,16 @@ r_vector<T>::proxy::proxy(SEXP data, const R_xlen_t index,
     : data_(data), index_(index), p_(p), is_altrep_(is_altrep) {}
 
 template <typename T>
-inline typename r_vector<T>::proxy& r_vector<T>::proxy::operator=(const T& rhs) {
-  underlying_type elt = static_cast<underlying_type>(rhs);
-
-  if (p_ != nullptr) {
-    *p_ = elt;
-  } else {
-    // Handles ALTREP, VECSXP, and STRSXP cases
-    set_elt(data_, index_, elt);
-  }
+inline typename r_vector<T>::proxy& r_vector<T>::proxy::operator=(const proxy& rhs) {
+  const underlying_type elt = rhs.get();
+  set(elt);
+  return *this;
+}
 
+template <typename T>
+inline typename r_vector<T>::proxy& r_vector<T>::proxy::operator=(const T& rhs) {
+  const underlying_type elt = static_cast<underlying_type>(rhs);
+  set(elt);
   return *this;
 }
 
@@ -1237,11 +1243,30 @@ inline void r_vector<T>::proxy::operator--() {
 
 template <typename T>
 inline r_vector<T>::proxy::operator T() const {
-  // Handles ALTREP, VECSXP, and STRSXP cases through `get_elt()`
-  const underlying_type elt = (p_ != nullptr) ? *p_ : r_vector::get_elt(data_, index_);
+  const underlying_type elt = get();
   return static_cast<T>(elt);
 }
 
+template <typename T>
+inline typename r_vector<T>::underlying_type r_vector<T>::proxy::get() const {
+  if (p_ != nullptr) {
+    return *p_;
+  } else {
+    // Handles ALTREP, VECSXP, and STRSXP cases
+    return r_vector::get_elt(data_, index_);
+  }
+}
+
+template <typename T>
+inline void r_vector<T>::proxy::set(typename r_vector<T>::underlying_type x) {
+  if (p_ != nullptr) {
+    *p_ = x;
+  } else {
+    // Handles ALTREP, VECSXP, and STRSXP cases
+    set_elt(data_, index_, x);
+  }
+}
+
 template <typename T>
 r_vector<T>::iterator::iterator(const r_vector* data, R_xlen_t pos)
     : r_vector::const_iterator(data, pos) {}