diff --git a/NEWS.md b/NEWS.md index 8ca9fa23..49b35efa 100644 --- a/NEWS.md +++ b/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). diff --git a/cpp11test/src/test-matrix.cpp b/cpp11test/src/test-matrix.cpp index 693d6ec7..39e33938 100644 --- a/cpp11test/src/test-matrix.cpp +++ b/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 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 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 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 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 x(5, 2); + expect_error(cpp11::writable::integers_matrix(x)); + } } diff --git a/cpp11test/src/test-r_vector.cpp b/cpp11test/src/test-r_vector.cpp index c73f48a8..9dc3f3c5 100644 --- a/cpp11test/src/test-r_vector.cpp +++ b/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. diff --git a/inst/include/cpp11/matrix.hpp b/inst/include/cpp11/matrix.hpp index 9a8454fe..8345068f 100644 --- a/inst/include/cpp11/matrix.hpp +++ b/inst/include/cpp11/matrix.hpp @@ -165,7 +165,8 @@ class matrix : public matrix_slices { matrix(SEXP data) : matrix_slices(data), vector_(data) {} template - matrix(const cpp11::matrix& rhs) : matrix_slices(rhs), vector_(rhs) {} + matrix(const cpp11::matrix& rhs) + : matrix_slices(rhs.nrow(), rhs.ncol()), vector_(rhs.vector()) {} matrix(int nrow, int ncol) : matrix_slices(nrow, ncol), vector_(R_xlen_t(nrow * ncol)) { @@ -179,6 +180,8 @@ class matrix : public matrix_slices { using matrix_slices::slice_stride; using matrix_slices::slice_offset; + V vector() const { return vector_; } + SEXP data() const { return vector_.data(); } R_xlen_t size() const { return vector_.size(); } diff --git a/inst/include/cpp11/r_vector.hpp b/inst/include/cpp11/r_vector.hpp index d548b390..66be7bd0 100644 --- a/inst/include/cpp11/r_vector.hpp +++ b/inst/include/cpp11/r_vector.hpp @@ -775,21 +775,21 @@ template inline r_vector::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 @@ -1279,13 +1279,14 @@ inline typename r_vector::iterator r_vector::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 inline SEXP r_vector::reserve_data(SEXP x, bool is_altrep, R_xlen_t size) { // Resize core data @@ -1294,7 +1295,9 @@ inline SEXP r_vector::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); }