From 6a2bc9b56aee8ef9e473e0eb92274a1589fb2f8d Mon Sep 17 00:00:00 2001 From: Davis Vaughan Date: Thu, 22 Aug 2024 16:40:06 -0400 Subject: [PATCH] Enable copy assignment operator for `iterator` class (#389) * Add capabilities tests * 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 * Include `` * NEWS bullets * Add iterator comments * Conditonally run capabilities tests It seems that the toolchain on 3.6 Windows did not have these implemented yet --- NEWS.md | 5 ++ cpp11test/src/test-r_vector.cpp | 100 ++++++++++++++++++++++++++++++++ inst/include/cpp11/r_vector.hpp | 31 +++++----- 3 files changed, 123 insertions(+), 13 deletions(-) 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). diff --git a/cpp11test/src/test-r_vector.cpp b/cpp11test/src/test-r_vector.cpp index 8f32a0f1..d138e998 100644 --- a/cpp11test/src/test-r_vector.cpp +++ b/cpp11test/src/test-r_vector.cpp @@ -4,6 +4,84 @@ #include +#include // for max_element + +#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; + + 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); + 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); + } +} +#endif + context("r_vector-C++") { test_that("writable vector temporary isn't leaked (integer) (#338)") { R_xlen_t before = cpp11::detail::store::count(); @@ -396,4 +474,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..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_; @@ -282,8 +288,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 +303,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 +1125,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 +1243,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 +1257,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;