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 `<algorithm>`

* NEWS bullets

* Add iterator comments

* Conditonally run capabilities tests

It seems that the toolchain on 3.6 Windows did not have these implemented yet

Author: DavisVaughan

NEWS.md

@@ -1,5 +1,10 @@
 # cpp11 (development version)
 
+* `cpp11::writable::r_vector<T>::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).
 

cpp11test/src/test-r_vector.cpp

@@ -4,6 +4,84 @@
 
 #include <testthat.h>
 
+#include <algorithm>  // 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<integers>::value);
+    expect_true(std::is_default_constructible<integers>::value);
+    expect_true(std::is_nothrow_default_constructible<integers>::value);
+    expect_true(std::is_copy_constructible<integers>::value);
+    expect_true(std::is_move_constructible<integers>::value);
+    expect_true(std::is_copy_assignable<integers>::value);
+    expect_true(std::is_move_assignable<integers>::value);
+  }
+
+  test_that("writable vector capabilities") {
+    using cpp11::writable::integers;
+
+    expect_true(std::is_destructible<integers>::value);
+    expect_true(std::is_default_constructible<integers>::value);
+    expect_true(std::is_nothrow_default_constructible<integers>::value);
+    expect_true(std::is_copy_constructible<integers>::value);
+    expect_true(std::is_move_constructible<integers>::value);
+    expect_true(std::is_copy_assignable<integers>::value);
+    expect_true(std::is_move_assignable<integers>::value);
+  }
+
+  test_that("read only const_iterator capabilities") {
+    using cpp11::integers;
+
+    expect_true(std::is_destructible<integers::const_iterator>::value);
+    expect_true(std::is_trivially_destructible<integers::const_iterator>::value);
+    expect_true(std::is_copy_constructible<integers::const_iterator>::value);
+    expect_true(std::is_move_constructible<integers::const_iterator>::value);
+    expect_true(std::is_copy_assignable<integers::const_iterator>::value);
+    expect_true(std::is_trivially_copy_assignable<integers::const_iterator>::value);
+    expect_true(std::is_move_assignable<integers::const_iterator>::value);
+    expect_true(std::is_trivially_move_assignable<integers::const_iterator>::value);
+  }
+
+  test_that("writable iterator capabilities") {
+    using cpp11::writable::integers;
+
+    expect_true(std::is_destructible<integers::iterator>::value);
+    expect_true(std::is_trivially_destructible<integers::iterator>::value);
+    expect_true(std::is_copy_constructible<integers::iterator>::value);
+    expect_true(std::is_move_constructible<integers::iterator>::value);
+    expect_true(std::is_copy_assignable<integers::iterator>::value);
+    expect_true(std::is_trivially_copy_assignable<integers::iterator>::value);
+    expect_true(std::is_move_assignable<integers::iterator>::value);
+    expect_true(std::is_trivially_move_assignable<integers::iterator>::value);
+  }
+
+  test_that("writable proxy capabilities") {
+    using cpp11::writable::integers;
+
+    expect_true(std::is_destructible<integers::proxy>::value);
+    expect_true(std::is_trivially_destructible<integers::proxy>::value);
+    expect_true(std::is_copy_constructible<integers::proxy>::value);
+    expect_true(std::is_move_constructible<integers::proxy>::value);
+
+    // Should these be true? Does it affect anything in practice?
+    expect_false(std::is_copy_assignable<integers::proxy>::value);
+    expect_false(std::is_trivially_copy_assignable<integers::proxy>::value);
+    expect_false(std::is_move_assignable<integers::proxy>::value);
+    expect_false(std::is_trivially_move_assignable<integers::proxy>::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);
+  }
 }

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<T> {
 
   class iterator : public cpp11::r_vector<T>::const_iterator {
    private:
-    const r_vector& data_;
-
+    using cpp11::r_vector<T>::const_iterator::data_;
     using cpp11::r_vector<T>::const_iterator::block_start_;
     using cpp11::r_vector<T>::const_iterator::pos_;
     using cpp11::r_vector<T>::const_iterator::buf_;
@@ -298,7 +303,7 @@ class r_vector : public cpp11::r_vector<T> {
     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<T>::clear() {
 
 template <typename T>
 inline typename r_vector<T>::iterator r_vector<T>::begin() const {
-  return iterator(*this, 0);
+  return iterator(this, 0);
 }
 
 template <typename T>
 inline typename r_vector<T>::iterator r_vector<T>::end() const {
-  return iterator(*this, length_);
+  return iterator(this, length_);
 }
 
 template <typename T>
@@ -1238,35 +1243,35 @@ inline r_vector<T>::proxy::operator T() const {
 }
 
 template <typename T>
-r_vector<T>::iterator::iterator(const r_vector& data, R_xlen_t pos)
-    : r_vector::const_iterator(&data, pos), data_(data) {}
+r_vector<T>::iterator::iterator(const r_vector* data, R_xlen_t pos)
+    : r_vector::const_iterator(data, pos) {}
 
 template <typename T>
 inline typename r_vector<T>::iterator& r_vector<T>::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;
 }
 
 template <typename T>
 inline typename r_vector<T>::proxy r_vector<T>::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<typename r_vector::underlying_type*>(&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 <typename T>
 inline typename r_vector<T>::iterator& r_vector<T>::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;