From 6d018391d79c08cbb05a3d860edbe15d2afdfb4d Mon Sep 17 00:00:00 2001 From: Davis Vaughan Date: Wed, 21 Aug 2024 14:07:55 -0400 Subject: [PATCH 1/4] Use `vector_(rhs.vector())` rather than `vector_(rhs)` The latter goes from `matrix -> SEXP -> r_vector` in a way that always avoids a copy, regardless of read-only or writable. This new way ensures that we go `r_vector -> r_vector`, and the correct `r_vector` constructor can get called. If its a writable input, it will be correctly copied now. --- cpp11test/src/test-matrix.cpp | 41 ++++++++++++++++++++++++++++++++++- inst/include/cpp11/matrix.hpp | 5 ++++- 2 files changed, 44 insertions(+), 2 deletions(-) diff --git a/cpp11test/src/test-matrix.cpp b/cpp11test/src/test-matrix.cpp index 693d6ec7..b356f4b2 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,43 @@ 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)); + + // TODO: Fix this + // // `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)); + + // TODO: Fix this + // // `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/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(); } From 1ea468f33db922d3ac64b955b6d5e2888de8f167 Mon Sep 17 00:00:00 2001 From: Davis Vaughan Date: Wed, 21 Aug 2024 14:13:55 -0400 Subject: [PATCH 2/4] NEWS bullet --- NEWS.md | 4 ++++ 1 file changed, 4 insertions(+) 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). From 989abb9e505085db7552128583eb888122ec4547 Mon Sep 17 00:00:00 2001 From: Davis Vaughan Date: Wed, 21 Aug 2024 14:35:42 -0400 Subject: [PATCH 3/4] Only resize names as required Otherwise treat it like other attributes that just get shallow copied over --- inst/include/cpp11/r_vector.hpp | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/inst/include/cpp11/r_vector.hpp b/inst/include/cpp11/r_vector.hpp index d548b390..9b9c382b 100644 --- a/inst/include/cpp11/r_vector.hpp +++ b/inst/include/cpp11/r_vector.hpp @@ -1294,7 +1294,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); } From ae7e2f61b94b8ecd39f4ecfcc5247770681d0c88 Mon Sep 17 00:00:00 2001 From: Davis Vaughan Date: Wed, 21 Aug 2024 14:39:10 -0400 Subject: [PATCH 4/4] Use `Rf_shallow_duplicate()` in `r_vector(const r_vector& rhs)` constructor after all The fact that `reserve_data()` drops `dim` and `dimnames` unconditionally is problematic here, because this copy constructor gets called from inside the `matrix` class. Really we should restrict usage of `reserve_data()` to JUST `reserve()`. This should be fine because it will only get called if we `push_back()`, and you can't `push_back()` on a `matrix`. --- cpp11test/src/test-matrix.cpp | 18 ++++++++---------- cpp11test/src/test-r_vector.cpp | 2 ++ inst/include/cpp11/r_vector.hpp | 33 +++++++++++++++++---------------- 3 files changed, 27 insertions(+), 26 deletions(-) diff --git a/cpp11test/src/test-matrix.cpp b/cpp11test/src/test-matrix.cpp index b356f4b2..39e33938 100644 --- a/cpp11test/src/test-matrix.cpp +++ b/cpp11test/src/test-matrix.cpp @@ -134,11 +134,10 @@ context("matrix-C++") { // Note that a copy should be made when copying writable! expect_true(SEXP(x) != SEXP(yr)); - // TODO: Fix this - // // `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); + // `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()); @@ -147,11 +146,10 @@ context("matrix-C++") { // Note that a copy should be made when copying writable! expect_true(SEXP(x) != SEXP(yc)); - // TODO: Fix this - // // `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); + // `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") { 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/r_vector.hpp b/inst/include/cpp11/r_vector.hpp index 9b9c382b..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