Merge pull request #387 from DavisVaughan/fix/clone-full-length

Shallow duplicate in `r_vector(const r_vector& rhs)` constructor after all

NEWS.md

@@ -1,5 +1,9 @@
 # cpp11 (development version)
 
+* Fixed an issue with the `writable::matrix` copy constructor where the
+  underlying SEXP should have been copied but was not. It is now consistent with
+  the behavior of the equivalent `writable::r_vector` copy constructor.
+
 * Added the missing implementation for `x.at("name")` for read only vectors
   (#370).
 

cpp11test/src/test-matrix.cpp

@@ -103,7 +103,7 @@ context("matrix-C++") {
     expect_true(xc(10, 13) == 121);
   }
 
-  test_that("copy constructor works") {
+  test_that("copy constructor works for read only matrices") {
     auto getExportedValue = cpp11::package("base")["getExportedValue"];
     cpp11::doubles_matrix<cpp11::by_row> x(getExportedValue("datasets", "volcano"));
 
@@ -119,4 +119,41 @@ context("matrix-C++") {
     expect_true(yc.nslices() == yc.ncol());
     expect_true(SEXP(x) == SEXP(yc));
   }
+
+  test_that("copy constructor works for writable matrices") {
+    cpp11::writable::doubles_matrix<cpp11::by_row> x(5, 2);
+
+    auto x_dim = x.attr("dim");
+    expect_true(INTEGER_ELT(x_dim, 0) == 5);
+    expect_true(INTEGER_ELT(x_dim, 1) == 2);
+
+    cpp11::writable::doubles_matrix<cpp11::by_row> yr(x);
+    expect_true(x.nrow() == yr.nrow());
+    expect_true(x.ncol() == yr.ncol());
+    expect_true(yr.nslices() == yr.nrow());
+    // Note that a copy should be made when copying writable!
+    expect_true(SEXP(x) != SEXP(yr));
+
+    // `dim` attribute is retained on copy
+    auto yr_dim = yr.attr("dim");
+    expect_true(INTEGER_ELT(yr_dim, 0) == 5);
+    expect_true(INTEGER_ELT(yr_dim, 1) == 2);
+
+    cpp11::writable::doubles_matrix<cpp11::by_column> yc(x);
+    expect_true(x.nrow() == yc.nrow());
+    expect_true(x.ncol() == yc.ncol());
+    expect_true(yc.nslices() == yc.ncol());
+    // Note that a copy should be made when copying writable!
+    expect_true(SEXP(x) != SEXP(yc));
+
+    // `dim` attribute is retained on copy
+    auto yc_dim = yc.attr("dim");
+    expect_true(INTEGER_ELT(yc_dim, 0) == 5);
+    expect_true(INTEGER_ELT(yc_dim, 1) == 2);
+  }
+
+  test_that("copy constructor is not enabled across vector types") {
+    cpp11::writable::doubles_matrix<cpp11::by_row> x(5, 2);
+    expect_error(cpp11::writable::integers_matrix<cpp11::by_column>(x));
+  }
 }

cpp11test/src/test-r_vector.cpp

@@ -281,10 +281,12 @@ context("r_vector-C++") {
 
     // Doubles the capacity from 2 to 4
     x.push_back(3);
+    expect_true(Rf_xlength(x.data()) == 4);
 
     // Calls writable copy constructor.
     // Should duplicate without truncations and retain same capacity.
     cpp11::writable::integers y(x);
+    expect_true(Rf_xlength(y.data()) == 4);
 
     // In the past, we truncated (i.e. to size 3) but retained the same capacity of 4,
     // so this could try to push without first resizing.

inst/include/cpp11/matrix.hpp

@@ -165,7 +165,8 @@ class matrix : public matrix_slices<S> {
   matrix(SEXP data) : matrix_slices<S>(data), vector_(data) {}
 
   template <typename V2, typename T2, typename S2>
-  matrix(const cpp11::matrix<V2, T2, S2>& rhs) : matrix_slices<S>(rhs), vector_(rhs) {}
+  matrix(const cpp11::matrix<V2, T2, S2>& rhs)
+      : matrix_slices<S>(rhs.nrow(), rhs.ncol()), vector_(rhs.vector()) {}
 
   matrix(int nrow, int ncol)
       : matrix_slices<S>(nrow, ncol), vector_(R_xlen_t(nrow * ncol)) {
@@ -179,6 +180,8 @@ class matrix : public matrix_slices<S> {
   using matrix_slices<S>::slice_stride;
   using matrix_slices<S>::slice_offset;
 
+  V vector() const { return vector_; }
+
   SEXP data() const { return vector_.data(); }
 
   R_xlen_t size() const { return vector_.size(); }

inst/include/cpp11/r_vector.hpp

@@ -775,21 +775,21 @@ template <typename T>
 inline r_vector<T>::r_vector(const r_vector& rhs) {
   // We don't want to just pass through to the read-only constructor because we'd
   // have to convert to `SEXP` first, which could truncate, and then we'd still have
-  // to shallow duplicate after that to ensure we have a duplicate, which can result in
-  // too many copies (#369).
+  // to shallow duplicate after that to really ensure we have a duplicate, which can
+  // result in too many copies (#369).
   //
   // Instead we take control of setting all fields to try and only duplicate 1 time.
-  // We try and reclaim unused capacity during the duplication by only reserving up to
-  // the `rhs.length_`. This is nice because if the user returns this object, the
-  // truncation has already been done and they don't have to pay for another allocation.
-  // Importantly, `reserve_data()` always duplicates even if there wasn't extra capacity,
-  // which ensures we have our own copy.
-  data_ = reserve_data(rhs.data_, rhs.is_altrep_, rhs.length_);
+  // If there is extra capacity in the `rhs`, that is also copied over. Resist the urge
+  // to try and trim the extra capacity during the duplication. We really do want to do a
+  // shallow duplicate to ensure that ALL attributes are copied over, including `dim` and
+  // `dimnames`, which would be lost if we instead used `reserve_data()` to do a combined
+  // duplicate + possible truncate. This is important for the `matrix` class.
+  data_ = safe[Rf_shallow_duplicate](rhs.data_);
   protect_ = detail::store::insert(data_);
   is_altrep_ = ALTREP(data_);
   data_p_ = get_p(is_altrep_, data_);
   length_ = rhs.length_;
-  capacity_ = rhs.length_;
+  capacity_ = rhs.capacity_;
 }
 
 template <typename T>
@@ -1279,13 +1279,14 @@ inline typename r_vector<T>::iterator r_vector<T>::iterator::operator+(R_xlen_t
   return it;
 }
 
-// Compared to `Rf_xlengthgets()`:
-// - This always allocates, even if it is the same size, which is important when we use
-//   it in a constructor and need to ensure that it duplicates on the way in.
-// - This copies over attributes with `Rf_copyMostAttrib()`, which is important when we
-//   use it in constructors and when we truncate right before returning from the `SEXP`
-//   operator.
-// - This is more friendly to ALTREP `x`.
+/// Compared to `Rf_xlengthgets()`:
+/// - This copies over attributes with `Rf_copyMostAttrib()`, which is important when we
+///   truncate right before returning from the `SEXP` operator.
+/// - This always allocates, even if it is the same size.
+/// - This is more friendly to ALTREP `x`.
+///
+/// SAFETY: For use only by `reserve()`! This won't retain the `dim` or `dimnames`
+/// attributes (which doesn't make much sense anyways).
 template <typename T>
 inline SEXP r_vector<T>::reserve_data(SEXP x, bool is_altrep, R_xlen_t size) {
   // Resize core data
@@ -1294,7 +1295,9 @@ inline SEXP r_vector<T>::reserve_data(SEXP x, bool is_altrep, R_xlen_t size) {
   // Resize names, if required
   SEXP names = Rf_getAttrib(x, R_NamesSymbol);
   if (names != R_NilValue) {
-    names = resize_names(names, size);
+    if (Rf_xlength(names) != size) {
+      names = resize_names(names, size);
+    }
     Rf_setAttrib(out, R_NamesSymbol, names);
   }