Replace Reader::ReadSomeDirectly() with Reader::ReadOrPullSome() …

…with a

different return value, indicating success rather than whether reading was
chosen over pulling. This is more consistent with the rest of the API. Checking
whether reading was chosen over pulling can be done by `length_read > 0` (on
success at least; on failure it does not matter which approach failed).

Fix `LimitingReaderBase::ReadOrPullSlow()`: check for the exact position
not reached when not enough was read because the underlying source ends or
failed, instead of when nothing could have been read because the limit was
reached before.

Fix `LimitingReaderBase::CopySlow()`: check for the exact position not reached
only when the underlying source ends or failed, instead of also when the
destination failed.

Cosmetics: in `Reader::ReadSomeSlow()` overloads with the `length_read`
parameter, do not use a separate path for setting `*length_read` in the failed
path if the successful path is at least as expensive. There is little point in
optimizing the failed path, and this avoids a conditional for the successful
path.
PiperOrigin-RevId: 604329564

riegeli/bytes/buffered_reader.cc

@@ -496,14 +496,14 @@ bool BufferedReader::CopySlow(size_t length, BackwardWriter& dest) {
   return dest.Write(std::move(data));
 }
 
-bool BufferedReader::ReadSomeDirectlySlow(
+bool BufferedReader::ReadOrPullSomeSlow(
     size_t max_length, absl::FunctionRef<char*(size_t&)> get_dest) {
   RIEGELI_ASSERT_GT(max_length, 0u)
-      << "Failed precondition of Reader::ReadSomeDirectlySlow(): "
-         "nothing to read, use ReadSomeDirectly() instead";
+      << "Failed precondition of Reader::ReadOrPullSomeSlow(): "
+         "nothing to read, use ReadOrPullSome() instead";
   RIEGELI_ASSERT_EQ(available(), 0u)
-      << "Failed precondition of Reader::ReadSomeDirectlySlow(): "
-         "some data available, use ReadSomeDirectly() instead";
+      << "Failed precondition of Reader::ReadOrPullSomeSlow(): "
+         "some data available, use ReadOrPullSome() instead";
   if (max_length >= buffer_sizer_.BufferLength(limit_pos())) {
     // Read directly to `get_dest(max_length)`.
     SyncBuffer();
@@ -516,12 +516,14 @@ bool BufferedReader::ReadSomeDirectlySlow(
       max_length = UnsignedMin(max_length, *exact_size() - limit_pos());
     }
     char* const dest = get_dest(max_length);
-    if (ABSL_PREDICT_FALSE(max_length == 0)) return true;
+    if (ABSL_PREDICT_FALSE(max_length == 0)) return false;
+    const Position pos_before = limit_pos();
     ReadInternal(ToleratesReadingAhead() ? max_length : 1, max_length, dest);
-    return true;
+    RIEGELI_ASSERT_GE(limit_pos(), pos_before)
+        << "BufferedReader::ReadInternal() decreased limit_pos()";
+    return limit_pos() > pos_before;
   }
-  PullSlow(1, max_length);
-  return false;
+  return PullSlow(1, max_length);
 }
 
 void BufferedReader::ReadHintSlow(size_t min_length,

riegeli/bytes/buffered_reader.h

@@ -165,9 +165,9 @@ class BufferedReader : public Reader {
   using Reader::CopySlow;
   bool CopySlow(Position length, Writer& dest) override;
   bool CopySlow(size_t length, BackwardWriter& dest) override;
-  using Reader::ReadSomeDirectlySlow;
-  bool ReadSomeDirectlySlow(
-      size_t max_length, absl::FunctionRef<char*(size_t&)> get_dest) override;
+  using Reader::ReadOrPullSomeSlow;
+  bool ReadOrPullSomeSlow(size_t max_length,
+                          absl::FunctionRef<char*(size_t&)> get_dest) override;
   void ReadHintSlow(size_t min_length, size_t recommended_length) override;
   bool SyncImpl(SyncType sync_type) override;
   bool SeekSlow(Position new_pos) override;

riegeli/bytes/joining_reader.cc

@@ -320,16 +320,16 @@ bool JoiningReaderBase::CopyBehindScratch(Position length, Writer& dest) {
   return true;
 }
 
-bool JoiningReaderBase::ReadSomeDirectlyBehindScratch(
+bool JoiningReaderBase::ReadOrPullSomeBehindScratch(
     size_t max_length, absl::FunctionRef<char*(size_t&)> get_dest) {
   RIEGELI_ASSERT_GT(max_length, 0u)
-      << "Failed precondition of Reader::ReadSomeDirectlyBehindScratch(): "
-         "nothing to read, use ReadSomeDirectly() instead";
+      << "Failed precondition of Reader::ReadOrPullSomeBehindScratch(): "
+         "nothing to read, use ReadOrPullSome() instead";
   RIEGELI_ASSERT_EQ(available(), 0u)
-      << "Failed precondition of Reader::ReadSomeDirectlyBehindScratch(): "
-         "some data available, use ReadSomeDirectly() instead";
+      << "Failed precondition of Reader::ReadOrPullSomeBehindScratch(): "
+         "some data available, use ReadOrPullSome() instead";
   RIEGELI_ASSERT(!scratch_used())
-      << "Failed precondition of Reader::ReadSomeDirectlyBehindScratch(): "
+      << "Failed precondition of Reader::ReadOrPullSomeBehindScratch(): "
          "scratch used";
   Reader* shard = ShardReader();
   if (shard_is_open(shard)) {
@@ -341,17 +341,22 @@ bool JoiningReaderBase::ReadSomeDirectlyBehindScratch(
   const Position remaining = std::numeric_limits<Position>::max() - limit_pos();
   if (ABSL_PREDICT_FALSE(remaining == 0)) return FailOverflow();
   max_length = UnsignedMin(max_length, remaining);
-  bool read_directly;
   for (;;) {
     size_t length_read;
-    read_directly = shard->ReadSomeDirectly(max_length, get_dest, &length_read);
-    if (read_directly) {
-      if (ABSL_PREDICT_TRUE(length_read > 0)) {
-        move_limit_pos(length_read);
-        break;
-      }
-    } else {
-      if (ABSL_PREDICT_TRUE(shard->available() > 0)) break;
+    bool write_ok = true;
+    const bool read_ok = shard->ReadOrPullSome(
+        max_length,
+        [get_dest, &write_ok](size_t& length) {
+          char* const dest = get_dest(length);
+          if (ABSL_PREDICT_FALSE(length == 0)) write_ok = false;
+          return dest;
+        },
+        &length_read);
+    move_limit_pos(length_read);
+    if (ABSL_PREDICT_TRUE(read_ok)) break;
+    if (ABSL_PREDICT_FALSE(!write_ok)) {
+      MakeBuffer(*shard);
+      return false;
     }
     if (ABSL_PREDICT_FALSE(!shard->ok())) {
       return FailWithoutAnnotation(AnnotateOverShard(shard->status()));
@@ -361,7 +366,7 @@ bool JoiningReaderBase::ReadSomeDirectlyBehindScratch(
     shard = ShardReader();
   }
   MakeBuffer(*shard);
-  return read_directly;
+  return true;
 }
 
 void JoiningReaderBase::ReadHintBehindScratch(size_t min_length,

riegeli/bytes/joining_reader.h

@@ -144,8 +144,8 @@ class JoiningReaderBase : public PullableReader {
   bool ReadBehindScratch(size_t length, absl::Cord& dest) override;
   using PullableReader::CopyBehindScratch;
   bool CopyBehindScratch(Position length, Writer& dest) override;
-  using PullableReader::ReadSomeDirectlyBehindScratch;
-  bool ReadSomeDirectlyBehindScratch(
+  using PullableReader::ReadOrPullSomeBehindScratch;
+  bool ReadOrPullSomeBehindScratch(
       size_t max_length, absl::FunctionRef<char*(size_t&)> get_dest) override;
   void ReadHintBehindScratch(size_t min_length,
                              size_t recommended_length) override;

riegeli/bytes/limiting_reader.cc

@@ -166,7 +166,10 @@ bool LimitingReaderBase::CopySlow(Position length, Writer& dest) {
   const Position length_to_copy = UnsignedMin(length, max_pos_ - pos());
   const bool copy_ok = src.Copy(length_to_copy, dest);
   MakeBuffer(src);
-  if (ABSL_PREDICT_FALSE(!copy_ok)) return CheckEnough();
+  if (ABSL_PREDICT_FALSE(!copy_ok)) {
+    if (dest.ok()) return CheckEnough();
+    return false;
+  }
   return length_to_copy == length;
 }
 
@@ -188,27 +191,38 @@ bool LimitingReaderBase::CopySlow(size_t length, BackwardWriter& dest) {
   }
   const bool copy_ok = src.Copy(length, dest);
   MakeBuffer(src);
-  if (ABSL_PREDICT_FALSE(!copy_ok)) return CheckEnough();
+  if (ABSL_PREDICT_FALSE(!copy_ok)) {
+    if (dest.ok()) return CheckEnough();
+    return false;
+  }
   return true;
 }
 
-bool LimitingReaderBase::ReadSomeDirectlySlow(
+bool LimitingReaderBase::ReadOrPullSomeSlow(
     size_t max_length, absl::FunctionRef<char*(size_t&)> get_dest) {
   RIEGELI_ASSERT_GT(max_length, 0u)
-      << "Failed precondition of Reader::ReadSomeDirectlySlow(): "
-         "nothing to read, use ReadSomeDirectly() instead";
+      << "Failed precondition of Reader::ReadOrPullSomeSlow(): "
+         "nothing to read, use ReadOrPullSome() instead";
   RIEGELI_ASSERT_EQ(available(), 0u)
-      << "Failed precondition of Reader::ReadSomeDirectlySlow(): "
-         "some data available, use ReadSomeDirectly() instead";
+      << "Failed precondition of Reader::ReadOrPullSomeSlow(): "
+         "some data available, use ReadOrPullSome() instead";
   if (ABSL_PREDICT_FALSE(!ok())) return false;
   Reader& src = *SrcReader();
   SyncBuffer(src);
-  const Position remaining = max_pos_ - pos();
-  const bool read_directly =
-      src.ReadSomeDirectly(UnsignedMin(max_length, remaining), get_dest);
+  const Position max_length_to_copy = UnsignedMin(max_length, max_pos_ - pos());
+  bool write_ok = true;
+  const bool read_ok = src.ReadOrPullSome(
+      max_length_to_copy, [get_dest, &write_ok](size_t& length) {
+        char* const dest = get_dest(length);
+        if (ABSL_PREDICT_FALSE(length == 0)) write_ok = false;
+        return dest;
+      });
   MakeBuffer(src);
-  if (ABSL_PREDICT_FALSE(remaining == 0)) return CheckEnough();
-  return read_directly;
+  if (ABSL_PREDICT_FALSE(!read_ok)) {
+    if (write_ok) return CheckEnough();
+    return false;
+  }
+  return max_length_to_copy > 0;
 }
 
 void LimitingReaderBase::ReadHintSlow(size_t min_length,

riegeli/bytes/limiting_reader.h

@@ -239,9 +239,9 @@ class LimitingReaderBase : public Reader {
   using Reader::CopySlow;
   bool CopySlow(Position length, Writer& dest) override;
   bool CopySlow(size_t length, BackwardWriter& dest) override;
-  using Reader::ReadSomeDirectlySlow;
-  bool ReadSomeDirectlySlow(
-      size_t max_length, absl::FunctionRef<char*(size_t&)> get_dest) override;
+  using Reader::ReadOrPullSomeSlow;
+  bool ReadOrPullSomeSlow(size_t max_length,
+                          absl::FunctionRef<char*(size_t&)> get_dest) override;
   void ReadHintSlow(size_t min_length, size_t recommended_length) override;
   bool SeekSlow(Position new_pos) override;
   absl::optional<Position> SizeImpl() override;

riegeli/bytes/position_shifting_reader.cc

@@ -157,20 +157,20 @@ bool PositionShiftingReaderBase::CopySlow(size_t length, BackwardWriter& dest) {
   return copy_ok;
 }
 
-bool PositionShiftingReaderBase::ReadSomeDirectlySlow(
+bool PositionShiftingReaderBase::ReadOrPullSomeSlow(
     size_t max_length, absl::FunctionRef<char*(size_t&)> get_dest) {
   RIEGELI_ASSERT_GT(max_length, 0u)
-      << "Failed precondition of Reader::ReadSomeDirectlySlow(): "
-         "nothing to read, use ReadSomeDirectly() instead";
+      << "Failed precondition of Reader::ReadOrPullSomeSlow(): "
+         "nothing to read, use ReadOrPullSome() instead";
   RIEGELI_ASSERT_EQ(available(), 0u)
-      << "Failed precondition of Reader::ReadSomeDirectlySlow(): "
-         "some data available, use ReadSomeDirectly() instead";
+      << "Failed precondition of Reader::ReadOrPullSomeSlow(): "
+         "some data available, use ReadOrPullSome() instead";
   if (ABSL_PREDICT_FALSE(!ok())) return false;
   Reader& src = *SrcReader();
   SyncBuffer(src);
-  const bool read_directly = src.ReadSomeDirectly(max_length, get_dest);
+  const bool read_ok = src.ReadOrPullSome(max_length, get_dest);
   MakeBuffer(src);
-  return read_directly;
+  return read_ok;
 }
 
 void PositionShiftingReaderBase::ReadHintSlow(size_t min_length,

riegeli/bytes/position_shifting_reader.h

@@ -109,9 +109,9 @@ class PositionShiftingReaderBase : public Reader {
   using Reader::CopySlow;
   bool CopySlow(Position length, Writer& dest) override;
   bool CopySlow(size_t length, BackwardWriter& dest) override;
-  using Reader::ReadSomeDirectlySlow;
-  bool ReadSomeDirectlySlow(
-      size_t max_length, absl::FunctionRef<char*(size_t&)> get_dest) override;
+  using Reader::ReadOrPullSomeSlow;
+  bool ReadOrPullSomeSlow(size_t max_length,
+                          absl::FunctionRef<char*(size_t&)> get_dest) override;
   void ReadHintSlow(size_t min_length, size_t recommended_length) override;
   bool SeekSlow(Position new_pos) override;
   absl::optional<Position> SizeImpl() override;

riegeli/bytes/prefix_limiting_reader.cc

@@ -146,20 +146,20 @@ bool PrefixLimitingReaderBase::CopySlow(size_t length, BackwardWriter& dest) {
   return copy_ok;
 }
 
-bool PrefixLimitingReaderBase::ReadSomeDirectlySlow(
+bool PrefixLimitingReaderBase::ReadOrPullSomeSlow(
     size_t max_length, absl::FunctionRef<char*(size_t&)> get_dest) {
   RIEGELI_ASSERT_GT(max_length, 0u)
-      << "Failed precondition of Reader::ReadSomeDirectlySlow(): "
-         "nothing to read, use ReadSomeDirectly() instead";
+      << "Failed precondition of Reader::ReadOrPullSomeSlow(): "
+         "nothing to read, use ReadOrPullSome() instead";
   RIEGELI_ASSERT_EQ(available(), 0u)
-      << "Failed precondition of Reader::ReadSomeDirectlySlow(): "
-         "some data available, use ReadSomeDirectly() instead";
+      << "Failed precondition of Reader::ReadOrPullSomeSlow(): "
+         "some data available, use ReadOrPullSome() instead";
   if (ABSL_PREDICT_FALSE(!ok())) return false;
   Reader& src = *SrcReader();
   SyncBuffer(src);
-  const bool read_directly = src.ReadSomeDirectly(max_length, get_dest);
+  const bool read_ok = src.ReadOrPullSome(max_length, get_dest);
   MakeBuffer(src);
-  return read_directly;
+  return read_ok;
 }
 
 void PrefixLimitingReaderBase::ReadHintSlow(size_t min_length,

riegeli/bytes/prefix_limiting_reader.h

@@ -108,9 +108,9 @@ class PrefixLimitingReaderBase : public Reader {
   using Reader::CopySlow;
   bool CopySlow(Position length, Writer& dest) override;
   bool CopySlow(size_t length, BackwardWriter& dest) override;
-  using Reader::ReadSomeDirectlySlow;
-  bool ReadSomeDirectlySlow(
-      size_t max_length, absl::FunctionRef<char*(size_t&)> get_dest) override;
+  using Reader::ReadOrPullSomeSlow;
+  bool ReadOrPullSomeSlow(size_t max_length,
+                          absl::FunctionRef<char*(size_t&)> get_dest) override;
   void ReadHintSlow(size_t min_length, size_t recommended_length) override;
   bool SeekSlow(Position new_pos) override;
   absl::optional<Position> SizeImpl() override;

riegeli/bytes/pullable_reader.cc

@@ -342,22 +342,21 @@ bool PullableReader::CopyBehindScratch(size_t length, BackwardWriter& dest) {
   return dest.Write(std::move(data));
 }
 
-bool PullableReader::ReadSomeDirectlyBehindScratch(
+bool PullableReader::ReadOrPullSomeBehindScratch(
     size_t max_length, absl::FunctionRef<char*(size_t&)> get_dest) {
   RIEGELI_ASSERT_GT(max_length, 0u)
       << "Failed precondition of "
-         "PullableReader::ReadSomeDirectlyBehindScratch(): "
-         "nothing to read, use ReadSomeDirectly() instead";
+         "PullableReader::ReadOrPullSomeBehindScratch(): "
+         "nothing to read, use ReadOrPullSome() instead";
   RIEGELI_ASSERT_EQ(available(), 0u)
       << "Failed precondition of "
-         "PullableReader::ReadSomeDirectlyBehindScratch(): "
-         "some data available, use ReadSomeDirectly() instead";
+         "PullableReader::ReadOrPullSomeBehindScratch(): "
+         "some data available, use ReadOrPullSome() instead";
   RIEGELI_ASSERT(!scratch_used())
       << "Failed precondition of "
-         "PullableReader::ReadSomeDirectlyBehindScratch(): "
+         "PullableReader::ReadOrPullSomeBehindScratch(): "
          "scratch used";
-  PullBehindScratch(max_length);
-  return false;
+  return PullBehindScratch(max_length);
 }
 
 void PullableReader::ReadHintBehindScratch(size_t min_length,
@@ -544,19 +543,19 @@ bool PullableReader::CopySlow(size_t length, BackwardWriter& dest) {
   return CopyBehindScratch(length, dest);
 }
 
-bool PullableReader::ReadSomeDirectlySlow(
+bool PullableReader::ReadOrPullSomeSlow(
     size_t max_length, absl::FunctionRef<char*(size_t&)> get_dest) {
   RIEGELI_ASSERT_GT(max_length, 0u)
-      << "Failed precondition of Reader::ReadSomeDirectlySlow(): "
-         "nothing to read, use ReadSomeDirectly() instead";
+      << "Failed precondition of Reader::ReadOrPullSomeSlow(): "
+         "nothing to read, use ReadOrPullSome() instead";
   RIEGELI_ASSERT_EQ(available(), 0u)
-      << "Failed precondition of Reader::ReadSomeDirectlySlow(): "
-         "some data available, use ReadSomeDirectly() instead";
+      << "Failed precondition of Reader::ReadOrPullSomeSlow(): "
+         "some data available, use ReadOrPullSome() instead";
   if (ABSL_PREDICT_FALSE(scratch_used())) {
     SyncScratch();
-    if (available() > 0) return false;
+    if (available() > 0) return true;
   }
-  return ReadSomeDirectlyBehindScratch(max_length, get_dest);
+  return ReadOrPullSomeBehindScratch(max_length, get_dest);
 }
 
 void PullableReader::ReadHintSlow(size_t min_length,

riegeli/bytes/pullable_reader.h

@@ -92,7 +92,7 @@ class PullableReader : public Reader {
   //   `!scratch_used()`
   virtual bool PullBehindScratch(size_t recommended_length) = 0;
 
-  // Implementation of `ReadSlow()`, `CopySlow()`, `ReadSomeDirectlySlow()`,
+  // Implementation of `ReadSlow()`, `CopySlow()`, `ReadOrPullSomeSlow()`,
   // `ReadHintSlow()`, `SyncImpl()`, and `SeekSlow()`, called while scratch is
   // not used.
   //
@@ -111,7 +111,7 @@ class PullableReader : public Reader {
   virtual bool ReadBehindScratch(size_t length, absl::Cord& dest);
   virtual bool CopyBehindScratch(Position length, Writer& dest);
   virtual bool CopyBehindScratch(size_t length, BackwardWriter& dest);
-  virtual bool ReadSomeDirectlyBehindScratch(
+  virtual bool ReadOrPullSomeBehindScratch(
       size_t max_length, absl::FunctionRef<char*(size_t&)> get_dest);
   virtual void ReadHintBehindScratch(size_t min_length,
                                      size_t recommended_length);
@@ -126,9 +126,9 @@ class PullableReader : public Reader {
   using Reader::CopySlow;
   bool CopySlow(Position length, Writer& dest) override;
   bool CopySlow(size_t length, BackwardWriter& dest) override;
-  using Reader::ReadSomeDirectlySlow;
-  bool ReadSomeDirectlySlow(
-      size_t max_length, absl::FunctionRef<char*(size_t&)> get_dest) override;
+  using Reader::ReadOrPullSomeSlow;
+  bool ReadOrPullSomeSlow(size_t max_length,
+                          absl::FunctionRef<char*(size_t&)> get_dest) override;
   void ReadHintSlow(size_t min_length, size_t recommended_length) override;
   bool SyncImpl(SyncType sync_type) override;
   bool SeekSlow(Position new_pos) override;