Improve repeated string fields:

- Add iteration support (eg `x.begin()` )
 - Add mutability support on indexing (eg `x[1] = "A";`)

PiperOrigin-RevId: 546017215

protos/repeated_field.h

@@ -71,9 +71,6 @@ class RepeatedFieldProxyBase {
   bool empty() const { return size() == 0; }
 
  protected:
-  // Returns upb_Array string member.
-  inline absl::string_view GetString(size_t n) const;
-
   // Returns upb_Array message member.
   inline upb_Message* GetMessage(size_t n) const;
 
@@ -81,13 +78,6 @@ class RepeatedFieldProxyBase {
   upb_Arena* arena_;
 };
 
-template <class T>
-inline absl::string_view RepeatedFieldProxyBase<T>::GetString(size_t n) const {
-  upb_MessageValue message_value = upb_Array_Get(arr_, n);
-  return absl::string_view(message_value.str_val.data,
-                           message_value.str_val.size);
-}
-
 template <class T>
 upb_Message* RepeatedFieldProxyBase<T>::GetMessage(size_t n) const {
   auto** messages =
@@ -174,6 +164,14 @@ class RepeatedFieldStringProxy
   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::Iterator<StringIteratorPolicy<T>>;
+  using reference = typename iterator::reference;
+  using pointer = typename iterator::pointer;
+  using reverse_iterator = std::reverse_iterator<iterator>;
+
   // Immutable constructor.
   explicit RepeatedFieldStringProxy(const upb_Array* arr, upb_Arena* arena)
       : RepeatedFieldProxyBase<T>(arr, arena) {}
@@ -183,7 +181,7 @@ class RepeatedFieldStringProxy
   // Constructor used by ::protos::Ptr.
   RepeatedFieldStringProxy(const RepeatedFieldStringProxy&) = default;
 
-  T operator[](size_t n) const { return this->GetString(n); }
+  reference operator[](size_t n) const { return begin()[n]; }
 
   template <int&... DeductionBlocker, bool b = !kIsConst,
             typename = std::enable_if_t<b>>
@@ -197,6 +195,13 @@ class RepeatedFieldStringProxy
     message_value.str_val = upb_StringView_FromDataAndSize(data, t.size());
     upb_Array_Append(this->arr_, message_value, this->arena_);
   }
+
+  iterator begin() const { return iterator({this->arr_, this->arena_, 0}); }
+  iterator end() const {
+    return iterator({this->arr_, this->arena_, this->size()});
+  }
+  reverse_iterator rbegin() const { return reverse_iterator(end()); }
+  reverse_iterator rend() const { return reverse_iterator(begin()); }
 };
 
 // RepeatedField proxy for repeated scalar types.
@@ -211,7 +216,7 @@ class RepeatedFieldScalarProxy
   using value_type = std::remove_const_t<T>;
   using size_type = size_t;
   using difference_type = ptrdiff_t;
-  using iterator = internal::RepeatedScalarIterator<T>;
+  using iterator = internal::Iterator<ScalarIteratorPolicy<T>>;
   using reference = typename iterator::reference;
   using pointer = typename iterator::pointer;
   using reverse_iterator = std::reverse_iterator<iterator>;
@@ -238,9 +243,9 @@ class RepeatedFieldScalarProxy
     upb_Array_Append(this->arr_, message_value, this->arena_);
   }
 
-  iterator begin() const { return iterator(unsafe_array()); }
+  iterator begin() const { return iterator({unsafe_array()}); }
   iterator cbegin() const { return begin(); }
-  iterator end() const { return iterator(unsafe_array() + this->size()); }
+  iterator end() const { return iterator({unsafe_array() + this->size()}); }
   iterator cend() const { return end(); }
 
   // Reverse iterator support.

protos/repeated_field_iterator.h

@@ -28,6 +28,7 @@
 #define UPB_PROTOS_REPEATED_FIELD_ITERATOR_H_
 
 #include <cstddef>
+#include <cstring>
 #include <iterator>
 #include <type_traits>
 
@@ -44,136 +45,197 @@
 namespace protos {
 namespace internal {
 
-// TODO(b/279086429): Implement std iterator for strings and messages
+// TODO(b/279086429): Implement std iterator for messages
 template <typename T>
 class RepeatedFieldScalarProxy;
+template <typename T>
+class RepeatedFieldStringProxy;
 
 struct IteratorTestPeer;
 
 template <typename T>
-class RepeatedScalarIterator;
+class Iterator;
+
+template <typename PolicyT>
+class ReferenceProxy;
+
+template <typename PolicyT>
+class InjectedRelationalsImpl {
+  using RP = ReferenceProxy<PolicyT>;
+  using V = typename PolicyT::value_type;
+  friend bool operator==(RP a, V b) { return static_cast<V>(a) == b; }
+  friend bool operator==(V a, RP b) { return a == static_cast<V>(b); }
+  friend bool operator==(RP a, RP b) {
+    return static_cast<V>(a) == static_cast<V>(b);
+  }
+  friend bool operator!=(RP a, V b) { return static_cast<V>(a) != b; }
+  friend bool operator!=(V a, RP b) { return a != static_cast<V>(b); }
+  friend bool operator!=(RP a, RP b) {
+    return static_cast<V>(a) != static_cast<V>(b);
+  }
+  friend bool operator<(RP a, V b) { return static_cast<V>(a) < b; }
+  friend bool operator<(V a, RP b) { return a < static_cast<V>(b); }
+  friend bool operator<(RP a, RP b) {
+    return static_cast<V>(a) < static_cast<V>(b);
+  }
+  friend bool operator<=(RP a, V b) { return static_cast<V>(a) <= b; }
+  friend bool operator<=(V a, RP b) { return a <= static_cast<V>(b); }
+  friend bool operator<=(RP a, RP b) {
+    return static_cast<V>(a) <= static_cast<V>(b);
+  }
+  friend bool operator>(RP a, V b) { return static_cast<V>(a) > b; }
+  friend bool operator>(V a, RP b) { return a > static_cast<V>(b); }
+  friend bool operator>(RP a, RP b) {
+    return static_cast<V>(a) > static_cast<V>(b);
+  }
+  friend bool operator>=(RP a, V b) { return static_cast<V>(a) >= b; }
+  friend bool operator>=(V a, RP b) { return a >= static_cast<V>(b); }
+  friend bool operator>=(RP a, RP b) {
+    return static_cast<V>(a) >= static_cast<V>(b);
+  }
+};
+class NoInjectedRelationalsImpl {};
+
+// We need to inject relationals for the string references because the
+// relationals for string_view are templates and won't allow for implicit
+// conversions from ReferenceProxy to string_view before deduction.
+template <typename PolicyT>
+using InjectedRelationals = std::conditional_t<
+    std::is_same_v<std::remove_const_t<typename PolicyT::value_type>,
+                   absl::string_view>,
+    InjectedRelationalsImpl<PolicyT>, NoInjectedRelationalsImpl>;
+
+template <typename PolicyT>
+class ReferenceProxy : InjectedRelationals<PolicyT> {
+  using value_type = typename PolicyT::value_type;
 
-template <typename T>
-class ReferenceProxy {
  public:
   ReferenceProxy(const ReferenceProxy&) = default;
   ReferenceProxy& operator=(const ReferenceProxy& other) {
     // Assign through the references
-    *ptr_ = *other.ptr_;
+    // TODO(sbenza): Make this better for strings to avoid the copy.
+    it_.Set(other.it_.Get());
     return *this;
   }
-  friend void swap(ReferenceProxy a, ReferenceProxy b) {
-    using std::swap;
-    swap(*a.ptr_, *b.ptr_);
-  }
+  friend void swap(ReferenceProxy a, ReferenceProxy b) { a.it_.swap(b.it_); }
 
-  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_);
-  }
+  operator value_type() const { return it_.Get(); }
+  void operator=(const value_type& value) const { it_.Set(value); }
+  void operator=(value_type&& value) const { it_.Set(std::move(value)); }
+  Iterator<PolicyT> operator&() const { return Iterator<PolicyT>(it_); }
 
  private:
   friend IteratorTestPeer;
-  friend ReferenceProxy<const T>;
-  friend RepeatedScalarIterator<T>;
+  friend ReferenceProxy<typename PolicyT::AddConst>;
+  friend Iterator<PolicyT>;
 
-  explicit ReferenceProxy(T& elem) : ptr_(&elem) {}
-  T* ptr_;
+  explicit ReferenceProxy(typename PolicyT::Payload elem) : it_(elem) {}
+  typename PolicyT::Payload it_;
 };
 
-template <typename T>
-class ReferenceProxy<const T> {
+template <template <typename> class PolicyTemplate, typename T>
+class ReferenceProxy<PolicyTemplate<const T>>
+    : InjectedRelationals<PolicyTemplate<const T>> {
+  using PolicyT = PolicyTemplate<const T>;
+  using value_type = typename PolicyT::value_type;
+
  public:
-  ReferenceProxy(ReferenceProxy<T> p) : ptr_(p.ptr_) {}
+  ReferenceProxy(ReferenceProxy<PolicyTemplate<T>> p) : it_(p.it_) {}
   ReferenceProxy(const ReferenceProxy&) = default;
   ReferenceProxy& operator=(const ReferenceProxy&) = delete;
 
-  operator T() const { return *ptr_; }
-  RepeatedScalarIterator<const T> operator&() const {
-    return RepeatedScalarIterator<const T>(ptr_);
-  }
+  operator value_type() const { return it_.Get(); }
+  Iterator<PolicyT> operator&() const { return Iterator<PolicyT>(it_); }
 
  private:
   friend IteratorTestPeer;
-  friend RepeatedScalarIterator<const T>;
+  friend Iterator<PolicyT>;
 
-  explicit ReferenceProxy(const T& ptr) : ptr_(&ptr) {}
-  const T* ptr_;
+  explicit ReferenceProxy(typename PolicyT::Payload elem) : it_(elem) {}
+  typename PolicyT::Payload it_;
 };
 
-template <typename T>
-class RepeatedScalarIterator {
+template <typename PolicyT>
+class Iterator {
  public:
   using iterator_category = std::random_access_iterator_tag;
-  using value_type = typename std::remove_const<T>::type;
+  using value_type = std::remove_const_t<typename PolicyT::value_type>;
   using difference_type = std::ptrdiff_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_) {}
-
-  constexpr reference operator*() const noexcept { return reference(*it_); }
+  using pointer = Iterator;
+  using reference = ReferenceProxy<PolicyT>;
+
+  constexpr Iterator() noexcept : it_(nullptr) {}
+  Iterator(const Iterator& other) = default;
+  Iterator& operator=(const Iterator& other) = default;
+  template <
+      typename P = PolicyT,
+      typename = std::enable_if_t<std::is_const<typename P::value_type>::value>>
+  Iterator(const Iterator<typename P::RemoveConst>& other) : it_(other.it_) {}
+
+  constexpr reference operator*() const noexcept { return reference(it_); }
   // No operator-> needed because T is a scalar.
 
  private:
   // Hide the internal type.
-  using iterator = RepeatedScalarIterator;
+  using iterator = Iterator;
 
  public:
   // {inc,dec}rementable
   constexpr iterator& operator++() noexcept {
-    ++it_;
+    it_.AddOffset(1);
     return *this;
   }
-  constexpr iterator operator++(int) noexcept { return iterator(it_++); }
+  constexpr iterator operator++(int) noexcept {
+    auto copy = *this;
+    ++*this;
+    return copy;
+  }
   constexpr iterator& operator--() noexcept {
-    --it_;
+    it_.AddOffset(-1);
     return *this;
   }
-  constexpr iterator operator--(int) noexcept { return iterator(it_--); }
+  constexpr iterator operator--(int) noexcept {
+    auto copy = *this;
+    --*this;
+    return copy;
+  }
 
   // equality_comparable
-  friend constexpr bool operator==(const iterator x,
-                                   const iterator y) noexcept {
-    return x.it_ == y.it_;
+  friend constexpr bool operator==(const iterator& x,
+                                   const iterator& y) noexcept {
+    return x.it_.Index() == y.it_.Index();
   }
-  friend constexpr bool operator!=(const iterator x,
-                                   const iterator y) noexcept {
-    return x.it_ != y.it_;
+  friend constexpr bool operator!=(const iterator& x,
+                                   const iterator& y) noexcept {
+    return !(x == y);
   }
 
   // less_than_comparable
-  friend constexpr bool operator<(const iterator x, const iterator y) noexcept {
-    return x.it_ < y.it_;
+  friend constexpr bool operator<(const iterator& x,
+                                  const iterator& y) noexcept {
+    return x.it_.Index() < y.it_.Index();
   }
-  friend constexpr bool operator<=(const iterator x,
-                                   const iterator y) noexcept {
-    return x.it_ <= y.it_;
+  friend constexpr bool operator<=(const iterator& x,
+                                   const iterator& y) noexcept {
+    return !(y < x);
   }
-  friend constexpr bool operator>(const iterator x, const iterator y) noexcept {
-    return x.it_ > y.it_;
+  friend constexpr bool operator>(const iterator& x,
+                                  const iterator& y) noexcept {
+    return y < x;
   }
-  friend constexpr bool operator>=(const iterator x,
-                                   const iterator y) noexcept {
-    return x.it_ >= y.it_;
+  friend constexpr bool operator>=(const iterator& x,
+                                   const iterator& y) noexcept {
+    return !(x < y);
   }
 
   constexpr iterator& operator+=(difference_type d) noexcept {
-    it_ += d;
+    it_.AddOffset(d);
     return *this;
   }
   constexpr iterator operator+(difference_type d) const noexcept {
-    return iterator(it_ + d);
+    auto copy = *this;
+    copy += d;
+    return copy;
   }
   friend constexpr iterator operator+(const difference_type d,
                                       iterator it) noexcept {
@@ -181,34 +243,108 @@ class RepeatedScalarIterator {
   }
 
   constexpr iterator& operator-=(difference_type d) noexcept {
-    it_ -= d;
+    it_.AddOffset(-d);
     return *this;
   }
   constexpr iterator operator-(difference_type d) const noexcept {
-    return iterator(it_ - d);
+    auto copy = *this;
+    copy -= d;
+    return copy;
   }
 
   // indexable
   constexpr reference operator[](difference_type d) const noexcept {
-    return reference(it_[d]);
+    auto copy = *this;
+    copy += d;
+    return *copy;
   }
 
   // random access iterator
   friend constexpr difference_type operator-(iterator x, iterator y) noexcept {
-    return x.it_ - y.it_;
+    return x.it_.Index() - y.it_.Index();
   }
 
  private:
   friend IteratorTestPeer;
-  friend ReferenceProxy<T>;
-  friend RepeatedScalarIterator<const T>;
-  friend class RepeatedFieldScalarProxy<T>;
+  friend ReferenceProxy<PolicyT>;
+  friend Iterator<typename PolicyT::AddConst>;
+  template <typename U>
+  friend class RepeatedFieldScalarProxy;
+  template <typename U>
+  friend class RepeatedFieldStringProxy;
 
   // Create from internal::RepeatedFieldScalarProxy.
-  explicit RepeatedScalarIterator(T* it) noexcept : it_(it) {}
+  explicit Iterator(typename PolicyT::Payload it) noexcept : it_(it) {}
 
   // The internal iterator.
-  T* it_;
+  typename PolicyT::Payload it_;
+};
+
+template <typename T>
+struct ScalarIteratorPolicy {
+  using value_type = T;
+  using RemoveConst = ScalarIteratorPolicy<std::remove_const_t<T>>;
+  using AddConst = ScalarIteratorPolicy<const T>;
+
+  struct Payload {
+    T* value;
+    void AddOffset(ptrdiff_t offset) { value += offset; }
+    T Get() const { return *value; }
+    void Set(T new_value) const { *value = new_value; }
+    T* Index() const { return value; }
+
+    void swap(Payload& other) {
+      using std::swap;
+      swap(*value, *other.value);
+    }
+
+    operator typename ScalarIteratorPolicy<const T>::Payload() const {
+      return {value};
+    }
+  };
+};
+
+template <typename T>
+struct StringIteratorPolicy {
+  using value_type = T;
+  using RemoveConst = StringIteratorPolicy<std::remove_const_t<T>>;
+  using AddConst = StringIteratorPolicy<const T>;
+
+  struct Payload {
+    using Array =
+        std::conditional_t<std::is_const_v<T>, const upb_Array, upb_Array>;
+    Array* arr;
+    upb_Arena* arena;
+    size_t index;
+
+    void AddOffset(ptrdiff_t offset) { index += offset; }
+    absl::string_view Get() const {
+      upb_MessageValue message_value = upb_Array_Get(arr, index);
+      return absl::string_view(message_value.str_val.data,
+                               message_value.str_val.size);
+    }
+    void Set(absl::string_view new_value) const {
+      char* data =
+          static_cast<char*>(upb_Arena_Malloc(arena, new_value.size()));
+      memcpy(data, new_value.data(), new_value.size());
+      upb_MessageValue message_value;
+      message_value.str_val =
+          upb_StringView_FromDataAndSize(data, new_value.size());
+      upb_Array_Set(arr, index, message_value);
+    }
+    size_t Index() const { return index; }
+
+    void swap(Payload& other) {
+      upb_MessageValue a = upb_Array_Get(this->arr, this->index);
+      upb_MessageValue b = upb_Array_Get(other.arr, other.index);
+      upb_Array_Set(this->arr, this->index, b);
+      upb_Array_Set(other.arr, other.index, a);
+    }
+
+    operator typename StringIteratorPolicy<const T>::Payload() const {
+      return {arr, arena, index};
+    }
+  };
 };
 
 }  // namespace internal

protos/repeated_field_iterator_test.cc

@@ -16,25 +16,46 @@ using ::testing::ElementsAre;
 namespace protos {
 namespace internal {
 
+template <typename T>
+using ScalarRef = ReferenceProxy<ScalarIteratorPolicy<T>>;
+template <typename T>
+using ScalarIterator = Iterator<ScalarIteratorPolicy<T>>;
+
+template <typename T>
+using StringRef = ReferenceProxy<StringIteratorPolicy<T>>;
+template <typename T>
+using StringIterator = Iterator<StringIteratorPolicy<T>>;
+
 struct IteratorTestPeer {
   template <typename T>
-  static ReferenceProxy<T> MakeRefProxy(T& ref) {
-    return ReferenceProxy<T>(ref);
+  static ScalarRef<T> MakeScalarRefProxy(T& ref) {
+    return ScalarRef<T>({&ref});
   }
 
   template <typename T>
-  static RepeatedScalarIterator<T> MakeIterator(T* ptr) {
-    return RepeatedScalarIterator<T>(ptr);
+  static ScalarIterator<T> MakeScalarIterator(T* ptr) {
+    return ScalarIterator<T>({ptr});
+  }
+
+  template <typename T>
+  static StringRef<T> MakeStringRefProxy(upb_Array* arr, protos::Arena& arena) {
+    return StringRef<T>({arr, arena.ptr(), 0});
+  }
+
+  template <typename T>
+  static StringIterator<T> MakeStringIterator(upb_Array* arr,
+                                              protos::Arena& arena) {
+    return StringIterator<T>({arr, arena.ptr()});
   }
 };
 
 namespace {
 
-TEST(ReferenceProxyTest, BasicOperationsWork) {
+TEST(ScalarReferenceTest, BasicOperationsWork) {
   int i = 0;
-  ReferenceProxy<int> p = IteratorTestPeer::MakeRefProxy(i);
-  ReferenceProxy<const int> cp =
-      IteratorTestPeer::MakeRefProxy(std::as_const(i));
+  ScalarRef<int> p = IteratorTestPeer::MakeScalarRefProxy(i);
+  ScalarRef<const int> cp =
+      IteratorTestPeer::MakeScalarRefProxy(std::as_const(i));
   EXPECT_EQ(i, 0);
   p = 17;
   EXPECT_EQ(i, 17);
@@ -47,17 +68,17 @@ TEST(ReferenceProxyTest, BasicOperationsWork) {
   EXPECT_FALSE((std::is_assignable<decltype(cp), int>::value));
 
   // Check that implicit conversion works T -> const T
-  ReferenceProxy<const int> cp2 = p;
+  ScalarRef<const int> cp2 = p;
   EXPECT_EQ(cp2, 13);
 
-  EXPECT_FALSE((std::is_convertible<decltype(cp), ReferenceProxy<int>>::value));
+  EXPECT_FALSE((std::is_convertible<decltype(cp), ScalarRef<int>>::value));
 }
 
-TEST(ReferenceProxyTest, AssignmentAndSwap) {
+TEST(ScalarReferenceTest, AssignmentAndSwap) {
   int i = 3;
   int j = 5;
-  ReferenceProxy<int> p = IteratorTestPeer::MakeRefProxy(i);
-  ReferenceProxy<int> p2 = IteratorTestPeer::MakeRefProxy(j);
+  ScalarRef<int> p = IteratorTestPeer::MakeScalarRefProxy(i);
+  ScalarRef<int> p2 = IteratorTestPeer::MakeScalarRefProxy(j);
 
   EXPECT_EQ(p, 3);
   EXPECT_EQ(p2, 5);
@@ -70,8 +91,8 @@ TEST(ReferenceProxyTest, AssignmentAndSwap) {
   EXPECT_EQ(p2, 3);
 }
 
-template <typename T>
-std::array<bool, 6> RunCompares(const T& a, const T& b) {
+template <typename T, typename U>
+std::array<bool, 6> RunCompares(const T& a, const U& b) {
   // Verify some basic properties here.
   // Equivalencies
   EXPECT_EQ((a == b), (b == a));
@@ -98,7 +119,7 @@ std::array<bool, 6> RunCompares(const T& a, const T& b) {
 
 template <typename T>
 void TestScalarIterator(T* array) {
-  RepeatedScalarIterator<T> it = IteratorTestPeer::MakeIterator(array);
+  ScalarIterator<T> it = IteratorTestPeer::MakeScalarIterator(array);
   // Copy
   auto it2 = it;
 
@@ -144,9 +165,9 @@ TEST(ScalarIteratorTest, BasicOperationsWork) {
 
 TEST(ScalarIteratorTest, Convertibility) {
   int array[10] = {10, 11, 12, 13, 14, 15, 16, 17, 18, 19};
-  RepeatedScalarIterator<int> it = IteratorTestPeer::MakeIterator(array);
+  ScalarIterator<int> it = IteratorTestPeer::MakeScalarIterator(array);
   it += 4;
-  RepeatedScalarIterator<const int> cit = it;
+  ScalarIterator<const int> cit = it;
   EXPECT_EQ(*it, 14);
   EXPECT_EQ(*cit, 14);
   it += 2;
@@ -156,36 +177,36 @@ TEST(ScalarIteratorTest, Convertibility) {
   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));
+  EXPECT_FALSE((std::is_convertible<ScalarIterator<const int>,
+                                    ScalarIterator<int>>::value));
+  EXPECT_FALSE((std::is_assignable<ScalarIterator<int>,
+                                   ScalarIterator<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);
+  ScalarIterator<int> it = IteratorTestPeer::MakeScalarIterator(array);
   EXPECT_EQ(array[3], 13);
   it[3] = 113;
   EXPECT_EQ(array[3], 113);
-  RepeatedScalarIterator<const int> cit = it;
+  ScalarIterator<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);
+  ScalarIterator<int> it = IteratorTestPeer::MakeScalarIterator(array);
   EXPECT_EQ(it[4], 14);
   // op& from references goes back to iterator.
-  RepeatedScalarIterator<int> it2 = &it[4];
+  ScalarIterator<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());
+  ScalarIterator<int> it = IteratorTestPeer::MakeScalarIterator(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));
@@ -197,6 +218,231 @@ TEST(ScalarIteratorTest, IteratorBasedAlgorithmsWork) {
   EXPECT_THAT(v, ElementsAre(10, 12, 14, 16, 18, 11, 13, 15, 17, 19));
 }
 
+const char* CloneString(protos::Arena& arena, absl::string_view str) {
+  char* data = (char*)upb_Arena_Malloc(arena.ptr(), str.size());
+  memcpy(data, str.data(), str.size());
+  return data;
+}
+upb_Array* MakeStringArray(protos::Arena& arena,
+                           const std::vector<std::string>& input) {
+  upb_Array* arr = upb_Array_New(arena.ptr(), kUpb_CType_String);
+  for (absl::string_view str : input) {
+    upb_MessageValue message_value;
+    message_value.str_val =
+        upb_StringView_FromDataAndSize(CloneString(arena, str), str.size());
+    upb_Array_Append(arr, message_value, arena.ptr());
+  }
+  return arr;
+}
+
+TEST(StringReferenceTest, BasicOperationsWork) {
+  protos::Arena arena;
+  upb_Array* arr = MakeStringArray(arena, {""});
+
+  auto read = [&] {
+    upb_MessageValue message_value = upb_Array_Get(arr, 0);
+    return absl::string_view(message_value.str_val.data,
+                             message_value.str_val.size);
+  };
+
+  StringRef<absl::string_view> p =
+      IteratorTestPeer::MakeStringRefProxy<absl::string_view>(arr, arena);
+  StringRef<const absl::string_view> cp =
+      IteratorTestPeer::MakeStringRefProxy<const absl::string_view>(arr, arena);
+  EXPECT_EQ(read(), "");
+  EXPECT_EQ(p, "");
+  p = "ABC";
+  EXPECT_EQ(read(), "ABC");
+  EXPECT_EQ(p, "ABC");
+  EXPECT_EQ(cp, "ABC");
+  const_cast<char*>(read().data())[0] = 'X';
+  EXPECT_EQ(read(), "XBC");
+  EXPECT_EQ(p, "XBC");
+  EXPECT_EQ(cp, "XBC");
+
+  EXPECT_FALSE((std::is_assignable<decltype(cp), int>::value));
+
+  // Check that implicit conversion works T -> const T
+  StringRef<const absl::string_view> cp2 = p;
+  EXPECT_EQ(cp2, "XBC");
+
+  EXPECT_FALSE(
+      (std::is_convertible<decltype(cp), StringRef<absl::string_view>>::value));
+
+  EXPECT_THAT(RunCompares(p, "XBC"),
+              ElementsAre(true, false, false, true, false, true));
+  EXPECT_THAT(RunCompares(p, "YBC"),
+              ElementsAre(false, true, true, true, false, false));
+  EXPECT_THAT(RunCompares(p, "RBC"),
+              ElementsAre(false, true, false, false, true, true));
+  EXPECT_THAT(RunCompares(p, "XB"),
+              ElementsAre(false, true, false, false, true, true));
+  EXPECT_THAT(RunCompares(p, "XBCD"),
+              ElementsAre(false, true, true, true, false, false));
+}
+
+TEST(StringReferenceTest, AssignmentAndSwap) {
+  protos::Arena arena;
+  upb_Array* arr1 = MakeStringArray(arena, {"ABC"});
+  upb_Array* arr2 = MakeStringArray(arena, {"DEF"});
+
+  auto p = IteratorTestPeer::MakeStringRefProxy<absl::string_view>(arr1, arena);
+  auto p2 =
+      IteratorTestPeer::MakeStringRefProxy<absl::string_view>(arr2, arena);
+
+  EXPECT_EQ(p, "ABC");
+  EXPECT_EQ(p2, "DEF");
+  swap(p, p2);
+  EXPECT_EQ(p, "DEF");
+  EXPECT_EQ(p2, "ABC");
+
+  p = p2;
+  EXPECT_EQ(p, "ABC");
+  EXPECT_EQ(p2, "ABC");
+}
+
+template <typename T>
+void TestStringIterator(protos::Arena& arena, upb_Array* array) {
+  StringIterator<T> it = IteratorTestPeer::MakeStringIterator<T>(array, arena);
+  // 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(StringIteratorTest, BasicOperationsWork) {
+  protos::Arena arena;
+  auto* array = MakeStringArray(
+      arena, {"10", "11", "12", "13", "14", "15", "16", "17", "18", "19"});
+  TestStringIterator<const absl::string_view>(arena, array);
+  TestStringIterator<absl::string_view>(arena, array);
+}
+
+TEST(StringIteratorTest, Convertibility) {
+  protos::Arena arena;
+  auto* array = MakeStringArray(
+      arena, {"10", "11", "12", "13", "14", "15", "16", "17", "18", "19"});
+  StringIterator<absl::string_view> it =
+      IteratorTestPeer::MakeStringIterator<absl::string_view>(array, arena);
+  it += 4;
+  StringIterator<const absl::string_view> 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<StringIterator<const absl::string_view>,
+                                    StringIterator<absl::string_view>>::value));
+  EXPECT_FALSE(
+      (std::is_assignable<StringIterator<absl::string_view>,
+                          StringIterator<const absl::string_view>>::value));
+}
+
+TEST(StringIteratorTest, MutabilityOnlyWorksOnMutable) {
+  protos::Arena arena;
+  auto* array = MakeStringArray(
+      arena, {"10", "11", "12", "13", "14", "15", "16", "17", "18", "19"});
+  StringIterator<absl::string_view> it =
+      IteratorTestPeer::MakeStringIterator<absl::string_view>(array, arena);
+
+  auto read = [&] {
+    upb_MessageValue message_value = upb_Array_Get(array, 3);
+    return absl::string_view(message_value.str_val.data,
+                             message_value.str_val.size);
+  };
+
+  EXPECT_EQ(read(), "13");
+  it[3] = "113";
+  EXPECT_EQ(read(), "113");
+  StringIterator<const absl::string_view> cit = it;
+  EXPECT_FALSE((std::is_assignable<decltype(*cit), absl::string_view>::value));
+  EXPECT_FALSE(
+      (std::is_assignable<decltype(cit[1]), absl::string_view>::value));
+}
+
+TEST(StringIteratorTest, IteratorReferenceInteraction) {
+  protos::Arena arena;
+  auto* array = MakeStringArray(
+      arena, {"10", "11", "12", "13", "14", "15", "16", "17", "18", "19"});
+  StringIterator<absl::string_view> it =
+      IteratorTestPeer::MakeStringIterator<absl::string_view>(array, arena);
+  EXPECT_EQ(it[4], "14");
+  // op& from references goes back to iterator.
+  StringIterator<absl::string_view> it2 = &it[4];
+  EXPECT_EQ(it + 4, it2);
+}
+
+TEST(StringIteratorTest, IteratorBasedAlgorithmsWork) {
+  protos::Arena arena;
+  auto* array = MakeStringArray(
+      arena, {"10", "11", "12", "13", "14", "15", "16", "17", "18", "19"});
+  StringIterator<absl::string_view> it =
+      IteratorTestPeer::MakeStringIterator<absl::string_view>(array, arena);
+
+  auto read = [&] {
+    std::vector<absl::string_view> v;
+    for (int i = 0; i < 10; ++i) {
+      upb_MessageValue message_value = upb_Array_Get(array, i);
+      v.emplace_back(message_value.str_val.data, message_value.str_val.size);
+    }
+    return v;
+  };
+
+  EXPECT_THAT(read(), ElementsAre("10", "11", "12", "13", "14",  //
+                                  "15", "16", "17", "18", "19"));
+  std::sort(it, it + 10, [](absl::string_view a, absl::string_view b) {
+    return std::tuple(a[1] % 2, a) < std::tuple(b[1] % 2, b);
+  });
+  EXPECT_THAT(read(), ElementsAre("10", "12", "14", "16", "18",  //
+                                  "11", "13", "15", "17", "19"));
+  // Now sort with the default less.
+  std::sort(it, it + 10);
+  EXPECT_THAT(read(), ElementsAre("10", "11", "12", "13", "14",  //
+                                  "15", "16", "17", "18", "19"));
+
+  // Mutable algorithm
+  std::generate(it, it + 10,
+                [i = 0]() mutable { return std::string(i++, 'x'); });
+  EXPECT_THAT(read(),
+              ElementsAre("", "x", "xx", "xxx", "xxxx", "xxxxx", "xxxxxx",
+                          "xxxxxxx", "xxxxxxxx", "xxxxxxxxx"));
+}
+
 }  // namespace
 }  // namespace internal
 }  // namespace protos

protos_generator/tests/test_generated.cc

@@ -515,10 +515,22 @@ TEST(CppGeneratedCode, RepeatedFieldProxyForStrings) {
   EXPECT_EQ(test_model.repeated_string()[1], "b");
   EXPECT_EQ(test_model.repeated_string()[2], "c");
 
+  EXPECT_THAT(test_model.repeated_string(), ElementsAre("a", "b", "c"));
+  EXPECT_THAT(*test_model.mutable_repeated_string(),
+              ElementsAre("a", "b", "c"));
+
   ASSERT_EQ(test_model.mutable_repeated_string()->size(), 3);
   EXPECT_EQ((*test_model.mutable_repeated_string())[0], "a");
   EXPECT_EQ((*test_model.mutable_repeated_string())[1], "b");
   EXPECT_EQ((*test_model.mutable_repeated_string())[2], "c");
+
+  // The const accessor can't be used to modify the element
+  EXPECT_FALSE((std::is_assignable<decltype(test_model.repeated_string()[1]),
+                                   absl::string_view>::value));
+  // But the mutable one is fine.
+  (*test_model.mutable_repeated_string())[1] = "other";
+  EXPECT_THAT(test_model.repeated_string(), ElementsAre("a", "other", "c"));
+
   test_model.mutable_repeated_string()->clear();
   EXPECT_EQ(test_model.mutable_repeated_string()->size(), 0);
 }