diff --git a/NEWS.md b/NEWS.md index 4475e522..410994f8 100644 --- a/NEWS.md +++ b/NEWS.md @@ -1,5 +1,9 @@ # cpp11 (development version) +* `cpp11::writable::r_vector::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 diff --git a/cpp11test/src/test-r_vector.cpp b/cpp11test/src/test-r_vector.cpp index d138e998..58c676b6 100644 --- a/cpp11test/src/test-r_vector.cpp +++ b/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 @@ -72,12 +73,8 @@ context("r_vector-capabilities-C++") { expect_true(std::is_trivially_destructible::value); expect_true(std::is_copy_constructible::value); expect_true(std::is_move_constructible::value); - - // Should these be true? Does it affect anything in practice? - expect_false(std::is_copy_assignable::value); - expect_false(std::is_trivially_copy_assignable::value); - expect_false(std::is_move_assignable::value); - expect_false(std::is_trivially_move_assignable::value); + expect_true(std::is_copy_assignable::value); + expect_true(std::is_move_assignable::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); diff --git a/inst/include/cpp11/r_vector.hpp b/inst/include/cpp11/r_vector.hpp index 28e4d5a3..f0bc2077 100644 --- a/inst/include/cpp11/r_vector.hpp +++ b/inst/include/cpp11/r_vector.hpp @@ -272,6 +272,8 @@ class r_vector : public cpp11::r_vector { 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 { void operator--(); operator T() const; + + private: + underlying_type get() const; + void set(underlying_type x); }; class iterator : public cpp11::r_vector::const_iterator { @@ -1176,16 +1182,16 @@ r_vector::proxy::proxy(SEXP data, const R_xlen_t index, : data_(data), index_(index), p_(p), is_altrep_(is_altrep) {} template -inline typename r_vector::proxy& r_vector::proxy::operator=(const T& rhs) { - underlying_type elt = static_cast(rhs); - - if (p_ != nullptr) { - *p_ = elt; - } else { - // Handles ALTREP, VECSXP, and STRSXP cases - set_elt(data_, index_, elt); - } +inline typename r_vector::proxy& r_vector::proxy::operator=(const proxy& rhs) { + const underlying_type elt = rhs.get(); + set(elt); + return *this; +} +template +inline typename r_vector::proxy& r_vector::proxy::operator=(const T& rhs) { + const underlying_type elt = static_cast(rhs); + set(elt); return *this; } @@ -1237,11 +1243,30 @@ inline void r_vector::proxy::operator--() { template inline r_vector::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(elt); } +template +inline typename r_vector::underlying_type r_vector::proxy::get() const { + if (p_ != nullptr) { + return *p_; + } else { + // Handles ALTREP, VECSXP, and STRSXP cases + return r_vector::get_elt(data_, index_); + } +} + +template +inline void r_vector::proxy::set(typename r_vector::underlying_type x) { + if (p_ != nullptr) { + *p_ = x; + } else { + // Handles ALTREP, VECSXP, and STRSXP cases + set_elt(data_, index_, x); + } +} + template r_vector::iterator::iterator(const r_vector* data, R_xlen_t pos) : r_vector::const_iterator(data, pos) {}