From 601a651cef37fddc156b25c0bd8aa3f69b5ff729 Mon Sep 17 00:00:00 2001 From: Davis Vaughan Date: Thu, 22 Aug 2024 13:59:12 -0400 Subject: [PATCH 1/6] Add capabilities tests --- cpp11test/src/test-r_vector.cpp | 66 +++++++++++++++++++++++++++++++++ 1 file changed, 66 insertions(+) diff --git a/cpp11test/src/test-r_vector.cpp b/cpp11test/src/test-r_vector.cpp index 8f32a0f1..553c285b 100644 --- a/cpp11test/src/test-r_vector.cpp +++ b/cpp11test/src/test-r_vector.cpp @@ -5,6 +5,72 @@ #include context("r_vector-C++") { + test_that("read only vector capabilities") { + using cpp11::integers; + + expect_true(std::is_destructible::value); + expect_true(std::is_default_constructible::value); + expect_true(std::is_nothrow_default_constructible::value); + expect_true(std::is_copy_constructible::value); + expect_true(std::is_move_constructible::value); + expect_true(std::is_copy_assignable::value); + expect_true(std::is_move_assignable::value); + } + + test_that("writable vector capabilities") { + using cpp11::writable::integers; + + expect_true(std::is_destructible::value); + expect_true(std::is_default_constructible::value); + expect_true(std::is_nothrow_default_constructible::value); + expect_true(std::is_copy_constructible::value); + expect_true(std::is_move_constructible::value); + expect_true(std::is_copy_assignable::value); + expect_true(std::is_move_assignable::value); + } + + test_that("read only const_iterator capabilities") { + using cpp11::integers; + + expect_true(std::is_destructible::value); + expect_true(std::is_trivially_destructible::value); + expect_true(std::is_copy_constructible::value); + expect_true(std::is_move_constructible::value); + expect_true(std::is_copy_assignable::value); + expect_true(std::is_trivially_copy_assignable::value); + expect_true(std::is_move_assignable::value); + expect_true(std::is_trivially_move_assignable::value); + } + + test_that("writable iterator capabilities") { + using cpp11::writable::integers; + + expect_true(std::is_destructible::value); + expect_true(std::is_trivially_destructible::value); + expect_true(std::is_copy_constructible::value); + expect_true(std::is_move_constructible::value); + // FIXME: These should be `true`! + // expect_true(std::is_copy_assignable::value); + // expect_true(std::is_trivially_copy_assignable::value); + // expect_true(std::is_move_assignable::value); + // expect_true(std::is_trivially_move_assignable::value); + } + + test_that("writable proxy capabilities") { + using cpp11::writable::integers; + + expect_true(std::is_destructible::value); + 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); + } + test_that("writable vector temporary isn't leaked (integer) (#338)") { R_xlen_t before = cpp11::detail::store::count(); From c60276895081838fbb833591b91dc0ac993a4166 Mon Sep 17 00:00:00 2001 From: Davis Vaughan Date: Thu, 22 Aug 2024 15:07:27 -0400 Subject: [PATCH 2/6] Use `data_` pointer from `const_iterator` This re-enables the copy assignment operator that was implicitly deleted for the `iterator` class, and also cleans some things up since we don't need the duplicate `data_` reference --- cpp11test/src/test-r_vector.cpp | 31 ++++++++++++++++++++++++++----- inst/include/cpp11/r_vector.hpp | 25 ++++++++++++------------- 2 files changed, 38 insertions(+), 18 deletions(-) diff --git a/cpp11test/src/test-r_vector.cpp b/cpp11test/src/test-r_vector.cpp index 553c285b..4ea193a4 100644 --- a/cpp11test/src/test-r_vector.cpp +++ b/cpp11test/src/test-r_vector.cpp @@ -49,11 +49,10 @@ context("r_vector-C++") { expect_true(std::is_trivially_destructible::value); expect_true(std::is_copy_constructible::value); expect_true(std::is_move_constructible::value); - // FIXME: These should be `true`! - // expect_true(std::is_copy_assignable::value); - // expect_true(std::is_trivially_copy_assignable::value); - // expect_true(std::is_move_assignable::value); - // expect_true(std::is_trivially_move_assignable::value); + expect_true(std::is_copy_assignable::value); + expect_true(std::is_trivially_copy_assignable::value); + expect_true(std::is_move_assignable::value); + expect_true(std::is_trivially_move_assignable::value); } test_that("writable proxy capabilities") { @@ -462,4 +461,26 @@ context("r_vector-C++") { 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); + SET_INTEGER_ELT(foo_sexp, 1, 2); + SET_INTEGER_ELT(foo_sexp, 2, 5); + SET_INTEGER_ELT(foo_sexp, 3, 4); + SET_INTEGER_ELT(foo_sexp, 4, 3); + cpp11::integers foo(foo_sexp); + + auto element = std::max_element(foo.begin(), foo.end()); + + expect_true(*element == 5); + + UNPROTECT(1); + } + + test_that("std::max_element works on writable vectors (#334)") { + cpp11::writable::integers foo = {1, 2, 5, 4, 3}; + auto element = std::max_element(foo.begin(), foo.end()); + expect_true(*element == 5); + } } diff --git a/inst/include/cpp11/r_vector.hpp b/inst/include/cpp11/r_vector.hpp index c87aa285..58fffd3e 100644 --- a/inst/include/cpp11/r_vector.hpp +++ b/inst/include/cpp11/r_vector.hpp @@ -282,8 +282,7 @@ class r_vector : public cpp11::r_vector { class iterator : public cpp11::r_vector::const_iterator { private: - const r_vector& data_; - + using cpp11::r_vector::const_iterator::data_; using cpp11::r_vector::const_iterator::block_start_; using cpp11::r_vector::const_iterator::pos_; using cpp11::r_vector::const_iterator::buf_; @@ -298,7 +297,7 @@ class r_vector : public cpp11::r_vector { using reference = proxy&; using iterator_category = std::forward_iterator_tag; - iterator(const r_vector& data, R_xlen_t pos); + iterator(const r_vector* data, R_xlen_t pos); iterator& operator++(); @@ -1120,12 +1119,12 @@ inline void r_vector::clear() { template inline typename r_vector::iterator r_vector::begin() const { - return iterator(*this, 0); + return iterator(this, 0); } template inline typename r_vector::iterator r_vector::end() const { - return iterator(*this, length_); + return iterator(this, length_); } template @@ -1238,13 +1237,13 @@ inline r_vector::proxy::operator T() const { } template -r_vector::iterator::iterator(const r_vector& data, R_xlen_t pos) - : r_vector::const_iterator(&data, pos), data_(data) {} +r_vector::iterator::iterator(const r_vector* data, R_xlen_t pos) + : r_vector::const_iterator(data, pos) {} template inline typename r_vector::iterator& r_vector::iterator::operator++() { ++pos_; - if (use_buf(data_.is_altrep()) && pos_ >= block_start_ + length_) { + if (use_buf(data_->is_altrep()) && pos_ >= block_start_ + length_) { fill_buf(pos_); } return *this; @@ -1252,21 +1251,21 @@ inline typename r_vector::iterator& r_vector::iterator::operator++() { template inline typename r_vector::proxy r_vector::iterator::operator*() const { - if (use_buf(data_.is_altrep())) { + if (use_buf(data_->is_altrep())) { return proxy( - data_.data(), pos_, + data_->data(), pos_, const_cast(&buf_[pos_ - block_start_]), true); } else { - return proxy(data_.data(), pos_, - data_.data_p_ != nullptr ? &data_.data_p_[pos_] : nullptr, false); + return proxy(data_->data(), pos_, + data_->data_p_ != nullptr ? &data_->data_p_[pos_] : nullptr, false); } } template inline typename r_vector::iterator& r_vector::iterator::operator+=(R_xlen_t rhs) { pos_ += rhs; - if (use_buf(data_.is_altrep()) && pos_ >= block_start_ + length_) { + if (use_buf(data_->is_altrep()) && pos_ >= block_start_ + length_) { fill_buf(pos_); } return *this; From 5b09a22541d77dda421da73e4025f432c1eebad6 Mon Sep 17 00:00:00 2001 From: Davis Vaughan Date: Thu, 22 Aug 2024 15:09:20 -0400 Subject: [PATCH 3/6] Include `` --- cpp11test/src/test-r_vector.cpp | 2 ++ 1 file changed, 2 insertions(+) diff --git a/cpp11test/src/test-r_vector.cpp b/cpp11test/src/test-r_vector.cpp index 4ea193a4..c50e0093 100644 --- a/cpp11test/src/test-r_vector.cpp +++ b/cpp11test/src/test-r_vector.cpp @@ -4,6 +4,8 @@ #include +#include // for max_element + context("r_vector-C++") { test_that("read only vector capabilities") { using cpp11::integers; From f0bf104b1fffcc8877f56b7aac58301618e4d641 Mon Sep 17 00:00:00 2001 From: Davis Vaughan Date: Thu, 22 Aug 2024 15:11:32 -0400 Subject: [PATCH 4/6] NEWS bullets --- NEWS.md | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/NEWS.md b/NEWS.md index 5d4fcf0e..6ae72571 100644 --- a/NEWS.md +++ b/NEWS.md @@ -1,5 +1,10 @@ # cpp11 (development version) +* `cpp11::writable::r_vector::iterator` no longer implicitly deletes its + copy assignment operator (#360). + +* `std::max_element()` can now be used with writable vectors (#334). + * The `environment` class no longer uses the non-API function `Rf_findVarInFrame3()` (#367). From 3c40a3ae72fc7abfb4ff7f498f3a6c31eca2af2e Mon Sep 17 00:00:00 2001 From: Davis Vaughan Date: Thu, 22 Aug 2024 15:54:42 -0400 Subject: [PATCH 5/6] Add iterator comments --- inst/include/cpp11/r_vector.hpp | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/inst/include/cpp11/r_vector.hpp b/inst/include/cpp11/r_vector.hpp index 58fffd3e..28e4d5a3 100644 --- a/inst/include/cpp11/r_vector.hpp +++ b/inst/include/cpp11/r_vector.hpp @@ -102,6 +102,12 @@ class r_vector { const_iterator find(const r_string& name) const; class const_iterator { + // Iterator references: + // https://cplusplus.com/reference/iterator/ + // https://stackoverflow.com/questions/8054273/how-to-implement-an-stl-style-iterator-and-avoid-common-pitfalls + // It seems like our iterator doesn't fully implement everything for + // `random_access_iterator_tag` (like an `[]` operator, for example). If we discover + // issues with it, we probably need to add more methods. private: const r_vector* data_; R_xlen_t pos_; From efa77339742553f8e226d443ffb8e439b34ff856 Mon Sep 17 00:00:00 2001 From: Davis Vaughan Date: Thu, 22 Aug 2024 16:18:10 -0400 Subject: [PATCH 6/6] Conditonally run capabilities tests It seems that the toolchain on 3.6 Windows did not have these implemented yet --- cpp11test/src/test-r_vector.cpp | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/cpp11test/src/test-r_vector.cpp b/cpp11test/src/test-r_vector.cpp index c50e0093..d138e998 100644 --- a/cpp11test/src/test-r_vector.cpp +++ b/cpp11test/src/test-r_vector.cpp @@ -6,7 +6,15 @@ #include // for max_element -context("r_vector-C++") { +#ifdef _WIN32 +#include "Rversion.h" +#define CPP11_HAS_IS_UTILITIES R_VERSION >= R_Version(4, 0, 0) +#else +#define CPP11_HAS_IS_UTILITIES 1 +#endif + +#if CPP11_HAS_IS_UTILITIES +context("r_vector-capabilities-C++") { test_that("read only vector capabilities") { using cpp11::integers; @@ -71,7 +79,10 @@ context("r_vector-C++") { expect_false(std::is_move_assignable::value); expect_false(std::is_trivially_move_assignable::value); } +} +#endif +context("r_vector-C++") { test_that("writable vector temporary isn't leaked (integer) (#338)") { R_xlen_t before = cpp11::detail::store::count();