pw_multibuf: AdvanceToData in iterator constructor

- Fix iteration over MultiBufs that start with an empty chunk.
- Make non-default iterator construction private to MultiBuf.
- Remove the unnecessary byte_index iterator constructor parameter.

Change-Id: I160514126cc822107d39c44c0110c5c7f86edf05
Reviewed-on: https://pigweed-review.googlesource.com/c/pigweed/pigweed/+/214503
Presubmit-Verified: CQ Bot Account <[email protected]>
Pigweed-Auto-Submit: Wyatt Hepler <[email protected]>
Lint: Lint 🤖 <[email protected]>
Reviewed-by: Taylor Cramer <[email protected]>
Commit-Queue: Auto-Submit <[email protected]>

pw_multibuf/multibuf.cc

@@ -247,12 +247,6 @@ MultiBuf::const_iterator& MultiBuf::const_iterator::operator+=(size_t advance) {
   return *this;
 }
 
-void MultiBuf::const_iterator::AdvanceToData() {
-  while (chunk_ != nullptr && chunk_->empty()) {
-    chunk_ = chunk_->next_in_buf_;
-  }
-}
-
 Chunk& MultiBuf::ChunkIterable::back() {
   return const_cast<Chunk&>(std::as_const(*this).back());
 }

pw_multibuf/multibuf_test.cc

@@ -407,6 +407,7 @@ TEST(MultiBuf, IteratorAdvancesNAcrossChunks) {
 TEST(MultiBuf, IteratorAdvancesNAcrossZeroLengthChunk) {
   AllocatorForTest<kArbitraryAllocatorSize> allocator;
   MultiBuf buf;
+  buf.PushBackChunk(MakeChunk(allocator, 0));
   buf.PushBackChunk(MakeChunk(allocator, {1_b, 2_b, 3_b}));
   buf.PushBackChunk(MakeChunk(allocator, 0));
   buf.PushBackChunk(MakeChunk(allocator, {4_b, 5_b, 6_b}));
@@ -427,5 +428,22 @@ TEST(MultiBuf, ConstIteratorAdvancesNAcrossChunks) {
   EXPECT_EQ(*iter, 5_b);
 }
 
+TEST(MultiBuf, IteratorSkipsEmptyChunks) {
+  AllocatorForTest<kArbitraryAllocatorSize> allocator;
+  MultiBuf buf;
+  buf.PushBackChunk(MakeChunk(allocator, 0));
+  buf.PushBackChunk(MakeChunk(allocator, 0));
+  buf.PushBackChunk(MakeChunk(allocator, {1_b}));
+  buf.PushBackChunk(MakeChunk(allocator, 0));
+  buf.PushBackChunk(MakeChunk(allocator, {2_b, 3_b}));
+  buf.PushBackChunk(MakeChunk(allocator, 0));
+
+  MultiBuf::iterator it = buf.begin();
+  ASSERT_EQ(*it++, 1_b);
+  ASSERT_EQ(*it++, 2_b);
+  ASSERT_EQ(*it++, 3_b);
+  ASSERT_EQ(it, buf.end());
+}
+
 }  // namespace
 }  // namespace pw::multibuf

pw_multibuf/public/pw_multibuf/multibuf.h

@@ -81,11 +81,11 @@ class MultiBuf {
   [[nodiscard]] bool empty() const;
 
   /// Returns an iterator pointing to the first byte of this ``MultiBuf`.
-  iterator begin() { return iterator(first_, 0); }
+  iterator begin() { return iterator(first_); }
   /// Returns a const iterator pointing to the first byte of this ``MultiBuf`.
-  const_iterator begin() const { return const_iterator(first_, 0); }
+  const_iterator begin() const { return const_iterator(first_); }
   /// Returns a const iterator pointing to the first byte of this ``MultiBuf`.
-  const_iterator cbegin() const { return const_iterator(first_, 0); }
+  const_iterator cbegin() const { return const_iterator(first_); }
 
   /// Returns an iterator pointing to the end of this ``MultiBuf``.
   iterator end() { return iterator::end(); }
@@ -257,10 +257,7 @@ class MultiBuf {
     using pointer = const std::byte*;
     using iterator_category = std::forward_iterator_tag;
 
-    constexpr const_iterator() = default;
-    const_iterator(const Chunk* chunk, size_t byte_index)
-        : chunk_(chunk), byte_index_(byte_index) {}
-    static const_iterator end() { return const_iterator(nullptr, 0); }
+    constexpr const_iterator() : chunk_(nullptr), byte_index_(0) {}
 
     reference operator*() const { return (*chunk_)[byte_index_]; }
     pointer operator->() const { return &(*chunk_)[byte_index_]; }
@@ -294,10 +291,23 @@ class MultiBuf {
     constexpr size_t byte_index() const { return byte_index_; }
 
    private:
-    void AdvanceToData();
+    friend class MultiBuf;
 
-    const Chunk* chunk_ = nullptr;
-    size_t byte_index_ = 0;
+    explicit constexpr const_iterator(const Chunk* chunk)
+        : chunk_(chunk), byte_index_(0) {
+      AdvanceToData();
+    }
+
+    static const_iterator end() { return const_iterator(nullptr); }
+
+    constexpr void AdvanceToData() {
+      while (chunk_ != nullptr && chunk_->empty()) {
+        chunk_ = chunk_->next_in_buf_;
+      }
+    }
+
+    const Chunk* chunk_;
+    size_t byte_index_;
   };
 
   /// An ``std::forward_iterator`` over the bytes of a ``MultiBuf``.
@@ -310,9 +320,6 @@ class MultiBuf {
     using iterator_category = std::forward_iterator_tag;
 
     constexpr iterator() = default;
-    iterator(Chunk* chunk, size_t byte_index)
-        : const_iter_(chunk, byte_index) {}
-    static iterator end() { return iterator(nullptr, 0); }
 
     reference operator*() const { return const_cast<std::byte&>(*const_iter_); }
     pointer operator->() const { return const_cast<std::byte*>(&*const_iter_); }
@@ -352,6 +359,12 @@ class MultiBuf {
     constexpr size_t byte_index() const { return const_iter_.byte_index(); }
 
    private:
+    friend class MultiBuf;
+
+    explicit constexpr iterator(Chunk* chunk) : const_iter_(chunk) {}
+
+    static iterator end() { return iterator(nullptr); }
+
     const_iterator const_iter_;
   };