Changes to scalar repeated field proxy:

- Add reference proxy type and change the scalar iterator to interact with it
 - Add missing Iterator<T> -> Iterator<const T> conversion
 - Add unit tests for reference and iterator types
 - Add missing typedefs
 - Add missing crbegin/crend functions

PiperOrigin-RevId: 544751376

protos/BUILD

@@ -139,3 +139,12 @@ upb_cc_proto_library_copts(
     copts = UPB_DEFAULT_CPPOPTS,
     visibility = ["//visibility:public"],
 )
+
+cc_test(
+    name = "repeated_field_iterator_test",
+    srcs = ["repeated_field_iterator_test.cc"],
+    deps = [
+        ":repeated_field",
+        "@com_google_googletest//:gtest_main",
+    ],
+)

protos/repeated_field.h

@@ -208,6 +208,14 @@ class RepeatedFieldScalarProxy
   static constexpr bool kIsConst = std::is_const_v<T>;
 
  public:
+  using value_type = std::remove_const_t<T>;
+  using size_type = size_t;
+  using difference_type = ptrdiff_t;
+  using iterator = internal::RepeatedScalarIterator<T>;
+  using reference = typename iterator::reference;
+  using pointer = typename iterator::pointer;
+  using reverse_iterator = std::reverse_iterator<iterator>;
+
   explicit RepeatedFieldScalarProxy(const upb_Array* arr, upb_Arena* arena)
       : RepeatedFieldProxyBase<T>(arr, arena) {}
   RepeatedFieldScalarProxy(upb_Array* arr, upb_Arena* arena)
@@ -230,25 +238,16 @@ class RepeatedFieldScalarProxy
     upb_Array_Append(this->arr_, message_value, this->arena_);
   }
 
-  // Iterator support.
-  using iterator = internal::RepeatedScalarIterator<T>;
-
   iterator begin() const { return iterator(unsafe_array()); }
   iterator cbegin() const { return begin(); }
   iterator end() const { return iterator(unsafe_array() + this->size()); }
   iterator cend() const { return end(); }
 
   // Reverse iterator support.
-  using const_reverse_iterator = std::reverse_iterator<iterator>;
-  using reverse_iterator = std::reverse_iterator<iterator>;
-  reverse_iterator rbegin() { return reverse_iterator(end()); }
-  const_reverse_iterator rbegin() const {
-    return const_reverse_iterator(end());
-  }
-  reverse_iterator rend() { return reverse_iterator(begin()); }
-  const_reverse_iterator rend() const {
-    return const_reverse_iterator(begin());
-  }
+  reverse_iterator rbegin() const { return reverse_iterator(end()); }
+  reverse_iterator rend() const { return reverse_iterator(begin()); }
+  reverse_iterator crbegin() const { return reverse_iterator(end()); }
+  reverse_iterator crend() const { return reverse_iterator(begin()); }
 
  private:
   T* unsafe_array() const {

protos/repeated_field_iterator.h

@@ -42,30 +42,88 @@
 #include "upb/port/def.inc"
 
 namespace protos {
-
 namespace internal {
 
 // TODO(b/279086429): Implement std iterator for strings and messages
 template <typename T>
 class RepeatedFieldScalarProxy;
 
+struct IteratorTestPeer;
+
+template <typename T>
+class RepeatedScalarIterator;
+
+template <typename T>
+class ReferenceProxy {
+ public:
+  ReferenceProxy(const ReferenceProxy&) = default;
+  ReferenceProxy& operator=(const ReferenceProxy& other) {
+    // Assign through the references
+    *ptr_ = *other.ptr_;
+    return *this;
+  }
+  friend void swap(ReferenceProxy a, ReferenceProxy b) {
+    using std::swap;
+    swap(*a.ptr_, *b.ptr_);
+  }
+
+  operator T() const { return *ptr_; }
+  void operator=(const T& value) const { *ptr_ = value; }
+  void operator=(T&& value) const { *ptr_ = std::move(value); }
+  RepeatedScalarIterator<T> operator&() const {
+    return RepeatedScalarIterator<T>(ptr_);
+  }
+
+ private:
+  friend IteratorTestPeer;
+  friend ReferenceProxy<const T>;
+  friend RepeatedScalarIterator<T>;
+
+  explicit ReferenceProxy(T& elem) : ptr_(&elem) {}
+  T* ptr_;
+};
+
+template <typename T>
+class ReferenceProxy<const T> {
+ public:
+  ReferenceProxy(ReferenceProxy<T> p) : ptr_(p.ptr_) {}
+  ReferenceProxy(const ReferenceProxy&) = default;
+  ReferenceProxy& operator=(const ReferenceProxy&) = delete;
+
+  operator T() const { return *ptr_; }
+  RepeatedScalarIterator<const T> operator&() const {
+    return RepeatedScalarIterator<const T>(ptr_);
+  }
+
+ private:
+  friend IteratorTestPeer;
+  friend RepeatedScalarIterator<const T>;
+
+  explicit ReferenceProxy(const T& ptr) : ptr_(&ptr) {}
+  const T* ptr_;
+};
+
 template <typename T>
 class RepeatedScalarIterator {
  public:
   using iterator_category = std::random_access_iterator_tag;
   using value_type = typename std::remove_const<T>::type;
   using difference_type = std::ptrdiff_t;
-  using pointer = T*;
-  using reference = T&;
+  using pointer = RepeatedScalarIterator;
+  using reference = ReferenceProxy<T>;
 
   constexpr RepeatedScalarIterator() noexcept : it_(nullptr) {}
   RepeatedScalarIterator(const RepeatedScalarIterator& other) = default;
   RepeatedScalarIterator& operator=(const RepeatedScalarIterator& other) =
       default;
+  template <typename U = T,
+            typename = std::enable_if_t<std::is_const<U>::value>>
+  RepeatedScalarIterator(
+      const RepeatedScalarIterator<std::remove_const_t<U>>& other)
+      : it_(other.it_) {}
 
-  // deref TODO(b/286450722) Change to use a proxy.
-  constexpr reference operator*() const noexcept { return *it_; }
-  constexpr pointer operator->() const noexcept { return it_; }
+  constexpr reference operator*() const noexcept { return reference(*it_); }
+  // No operator-> needed because T is a scalar.
 
  private:
   // Hide the internal type.
@@ -132,7 +190,7 @@ class RepeatedScalarIterator {
 
   // indexable
   constexpr reference operator[](difference_type d) const noexcept {
-    return it_[d];
+    return reference(it_[d]);
   }
 
   // random access iterator
@@ -141,6 +199,9 @@ class RepeatedScalarIterator {
   }
 
  private:
+  friend IteratorTestPeer;
+  friend ReferenceProxy<T>;
+  friend RepeatedScalarIterator<const T>;
   friend class RepeatedFieldScalarProxy<T>;
 
   // Create from internal::RepeatedFieldScalarProxy.
@@ -151,7 +212,6 @@ class RepeatedScalarIterator {
 };
 
 }  // namespace internal
-
 }  // namespace protos
 
 #endif  // UPB_PROTOS_REPEATED_FIELD_ITERATOR_H_

protos/repeated_field_iterator_test.cc

@@ -0,0 +1,202 @@
+#include "protos/repeated_field_iterator.h"
+
+#include <algorithm>
+#include <array>
+#include <numeric>
+#include <tuple>
+#include <type_traits>
+#include <utility>
+#include <vector>
+
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+
+using ::testing::ElementsAre;
+
+namespace protos {
+namespace internal {
+
+struct IteratorTestPeer {
+  template <typename T>
+  static ReferenceProxy<T> MakeRefProxy(T& ref) {
+    return ReferenceProxy<T>(ref);
+  }
+
+  template <typename T>
+  static RepeatedScalarIterator<T> MakeIterator(T* ptr) {
+    return RepeatedScalarIterator<T>(ptr);
+  }
+};
+
+namespace {
+
+TEST(ReferenceProxyTest, BasicOperationsWork) {
+  int i = 0;
+  ReferenceProxy<int> p = IteratorTestPeer::MakeRefProxy(i);
+  ReferenceProxy<const int> cp =
+      IteratorTestPeer::MakeRefProxy(std::as_const(i));
+  EXPECT_EQ(i, 0);
+  p = 17;
+  EXPECT_EQ(i, 17);
+  EXPECT_EQ(p, 17);
+  EXPECT_EQ(cp, 17);
+  i = 13;
+  EXPECT_EQ(p, 13);
+  EXPECT_EQ(cp, 13);
+
+  EXPECT_FALSE((std::is_assignable<decltype(cp), int>::value));
+
+  // Check that implicit conversion works T -> const T
+  ReferenceProxy<const int> cp2 = p;
+  EXPECT_EQ(cp2, 13);
+
+  EXPECT_FALSE((std::is_convertible<decltype(cp), ReferenceProxy<int>>::value));
+}
+
+TEST(ReferenceProxyTest, AssignmentAndSwap) {
+  int i = 3;
+  int j = 5;
+  ReferenceProxy<int> p = IteratorTestPeer::MakeRefProxy(i);
+  ReferenceProxy<int> p2 = IteratorTestPeer::MakeRefProxy(j);
+
+  EXPECT_EQ(p, 3);
+  EXPECT_EQ(p2, 5);
+  swap(p, p2);
+  EXPECT_EQ(p, 5);
+  EXPECT_EQ(p2, 3);
+
+  p = p2;
+  EXPECT_EQ(p, 3);
+  EXPECT_EQ(p2, 3);
+}
+
+template <typename T>
+std::array<bool, 6> RunCompares(const T& a, const T& b) {
+  // Verify some basic properties here.
+  // Equivalencies
+  EXPECT_EQ((a == b), (b == a));
+  EXPECT_EQ((a != b), (b != a));
+  EXPECT_EQ((a < b), (b > a));
+  EXPECT_EQ((a > b), (b < a));
+  EXPECT_EQ((a <= b), (b >= a));
+  EXPECT_EQ((a >= b), (b <= a));
+
+  // Opposites
+  EXPECT_NE((a == b), (a != b));
+  EXPECT_NE((a < b), (a >= b));
+  EXPECT_NE((a > b), (a <= b));
+
+  return {{
+      (a == b),
+      (a != b),
+      (a < b),
+      (a <= b),
+      (a > b),
+      (a >= b),
+  }};
+}
+
+template <typename T>
+void TestScalarIterator(T* array) {
+  RepeatedScalarIterator<T> it = IteratorTestPeer::MakeIterator(array);
+  // Copy
+  auto it2 = it;
+
+  EXPECT_THAT(RunCompares(it, it2),
+              ElementsAre(true, false, false, true, false, true));
+
+  // Increment
+  EXPECT_EQ(*++it, 11);
+  EXPECT_EQ(*it2, 10);
+  EXPECT_EQ(*it++, 11);
+  EXPECT_EQ(*it2, 10);
+  EXPECT_EQ(*it, 12);
+  EXPECT_EQ(*it2, 10);
+
+  EXPECT_THAT(RunCompares(it, it2),
+              ElementsAre(false, true, false, false, true, true));
+
+  // Assign
+  it2 = it;
+  EXPECT_EQ(*it, 12);
+  EXPECT_EQ(*it2, 12);
+
+  // Decrement
+  EXPECT_EQ(*--it, 11);
+  EXPECT_EQ(*it--, 11);
+  EXPECT_EQ(*it, 10);
+
+  it += 5;
+  EXPECT_EQ(*it, 15);
+  EXPECT_EQ(it - it2, 3);
+  EXPECT_EQ(it2 - it, -3);
+  it -= 3;
+  EXPECT_EQ(*it, 12);
+  EXPECT_EQ(it[6], 18);
+  EXPECT_EQ(it[-1], 11);
+}
+
+TEST(ScalarIteratorTest, BasicOperationsWork) {
+  int array[10] = {10, 11, 12, 13, 14, 15, 16, 17, 18, 19};
+  TestScalarIterator<const int>(array);
+  TestScalarIterator<int>(array);
+}
+
+TEST(ScalarIteratorTest, Convertibility) {
+  int array[10] = {10, 11, 12, 13, 14, 15, 16, 17, 18, 19};
+  RepeatedScalarIterator<int> it = IteratorTestPeer::MakeIterator(array);
+  it += 4;
+  RepeatedScalarIterator<const int> cit = it;
+  EXPECT_EQ(*it, 14);
+  EXPECT_EQ(*cit, 14);
+  it += 2;
+  EXPECT_EQ(*it, 16);
+  EXPECT_EQ(*cit, 14);
+  cit = it;
+  EXPECT_EQ(*it, 16);
+  EXPECT_EQ(*cit, 16);
+
+  EXPECT_FALSE((std::is_convertible<RepeatedScalarIterator<const int>,
+                                    RepeatedScalarIterator<int>>::value));
+  EXPECT_FALSE((std::is_assignable<RepeatedScalarIterator<int>,
+                                   RepeatedScalarIterator<const int>>::value));
+}
+
+TEST(ScalarIteratorTest, MutabilityOnlyWorksOnMutable) {
+  int array[10] = {10, 11, 12, 13, 14, 15, 16, 17, 18, 19};
+  RepeatedScalarIterator<int> it = IteratorTestPeer::MakeIterator(array);
+  EXPECT_EQ(array[3], 13);
+  it[3] = 113;
+  EXPECT_EQ(array[3], 113);
+  RepeatedScalarIterator<const int> cit = it;
+  EXPECT_FALSE((std::is_assignable<decltype(*cit), int>::value));
+  EXPECT_FALSE((std::is_assignable<decltype(cit[1]), int>::value));
+}
+
+TEST(ScalarIteratorTest, IteratorReferenceInteraction) {
+  int array[10] = {10, 11, 12, 13, 14, 15, 16, 17, 18, 19};
+  RepeatedScalarIterator<int> it = IteratorTestPeer::MakeIterator(array);
+  EXPECT_EQ(it[4], 14);
+  // op& from references goes back to iterator.
+  RepeatedScalarIterator<int> it2 = &it[4];
+  EXPECT_EQ(it + 4, it2);
+}
+
+TEST(ScalarIteratorTest, IteratorBasedAlgorithmsWork) {
+  // We use a vector here to make testing it easier.
+  std::vector<int> v(10, 0);
+  RepeatedScalarIterator<int> it = IteratorTestPeer::MakeIterator(v.data());
+  EXPECT_THAT(v, ElementsAre(0, 0, 0, 0, 0, 0, 0, 0, 0, 0));
+  std::iota(it, it + 10, 10);
+  EXPECT_THAT(v, ElementsAre(10, 11, 12, 13, 14, 15, 16, 17, 18, 19));
+  EXPECT_EQ(it + 5, std::find(it, it + 10, 15));
+  EXPECT_EQ(145, std::accumulate(it, it + 10, 0));
+  std::sort(it, it + 10, [](int a, int b) {
+    return std::tuple(a % 2, a) < std::tuple(b % 2, b);
+  });
+  EXPECT_THAT(v, ElementsAre(10, 12, 14, 16, 18, 11, 13, 15, 17, 19));
+}
+
+}  // namespace
+}  // namespace internal
+}  // namespace protos

protos_generator/tests/test_generated.cc

@@ -27,7 +27,9 @@
 #include <memory>
 #include <string>
 #include <utility>
+#include <vector>
 
+#include "gmock/gmock.h"
 #include "gtest/gtest.h"
 #include "absl/status/statusor.h"
 #include "absl/strings/string_view.h"
@@ -50,6 +52,7 @@ using ::protos_generator::test::protos::TestModel_Category_NEWS;
 using ::protos_generator::test::protos::TestModel_Category_VIDEO;
 using ::protos_generator::test::protos::theme;
 using ::protos_generator::test::protos::ThemeExtension;
+using ::testing::ElementsAre;
 
 TEST(CppGeneratedCode, Constructor) { TestModel test_model; }
 
@@ -412,10 +415,22 @@ TEST(CppGeneratedCode, RepeatedFieldProxyForScalars) {
   EXPECT_EQ((*test_model.mutable_value_array())[1], 16);
   EXPECT_EQ((*test_model.mutable_value_array())[2], 27);
 
-  ASSERT_EQ(test_model.value_array().size(), 3);
-  EXPECT_EQ(test_model.value_array()[0], 5);
-  EXPECT_EQ(test_model.value_array()[1], 16);
-  EXPECT_EQ(test_model.value_array()[2], 27);
+  const auto value_array = test_model.value_array();
+  ASSERT_EQ(value_array.size(), 3);
+  EXPECT_EQ(value_array[0], 5);
+  EXPECT_EQ(value_array[1], 16);
+  EXPECT_EQ(value_array[2], 27);
+
+  EXPECT_THAT(value_array, ElementsAre(5, 16, 27));
+
+  EXPECT_THAT(std::vector(value_array.begin(), value_array.end()),
+              ElementsAre(5, 16, 27));
+  EXPECT_THAT(std::vector(value_array.cbegin(), value_array.cend()),
+              ElementsAre(5, 16, 27));
+  EXPECT_THAT(std::vector(value_array.rbegin(), value_array.rend()),
+              ElementsAre(27, 16, 5));
+  EXPECT_THAT(std::vector(value_array.crbegin(), value_array.crend()),
+              ElementsAre(27, 16, 5));
 }
 
 TEST(CppGeneratedCode, RepeatedScalarIterator) {