What do you think about this diff (based on d60fa16)? I didn't like the fact that FillBufferUpToLength() would call Fill() but would not progress m_read_pos. With this change, (what's now called) ReadBufferOnceUpToLength() both progress m_read_pos and calls Fill() if necessary, without doing any looping.
Besides feeling more intuitive (to me), it also allows for some more code cleanup. My main concern is that in read() we now progress m_read_pos before copying it to dst, so if something fails there we'd need to rewind the buffer. I don't actually see that be a problem, but wanted to mention it in case I'm missing something?
Oh and I reverted everything back to uint64_t, since those are the types used for CBufferedFile's relevant members? Hopefully that'll fix the CI issues, but I'm a bit out of my depth here.
<details>
diff --git a/src/streams.h b/src/streams.h
index 23e617e56..dbc6e4c03 100644
--- a/src/streams.h
+++ b/src/streams.h
@@ -629,15 +629,18 @@ private:
return true;
}
- //! Make up to the requested 'length' (but at least 1) bytes available
- //! in the buffer, starting at stream offset 'm_read_pos', filling the
- //! buffer if no bytes are available.
- //! Return the (possibly lower) number of bytes actually available.
- size_t FillBufferUpToLength(size_t buffer_offset, size_t length)
+ //! Read up to the requested 'length' bytes in the buffer, starting at stream offset
+ //! 'm_read_pos', filling the buffer if no bytes are available.
+ //! Return how many bytes were read in this iteration, which may be less than 'length'.
+ uint64_t ReadBufferOnceUpToLength(const uint64_t length)
{
assert(m_read_pos <= nSrcPos);
- if (m_read_pos == nSrcPos) Fill();
- return std::min({length, vchBuf.size() - buffer_offset, static_cast<size_t>(nSrcPos - m_read_pos)});
+ if (m_read_pos == nSrcPos && length > 0) Fill();
+ uint64_t advance{std::min({length, // bytes requested
+ vchBuf.size() - (m_read_pos % vchBuf.size()), // bytes until buffer end
+ nSrcPos - m_read_pos})}; // bytes not yet read
+ m_read_pos += advance;
+ return advance;
}
public:
@@ -680,12 +683,10 @@ public:
if (dst.size() + m_read_pos > nReadLimit) {
throw std::ios_base::failure("Read attempted past buffer limit");
}
- while (dst.size() > 0) {
- uint64_t pos{m_read_pos % vchBuf.size()};
- size_t length{FillBufferUpToLength(pos, dst.size())};
- memcpy(dst.data(), &vchBuf[pos], length);
- dst = dst.subspan(length);
- m_read_pos += length;
+ while (uint64_t bytes_read{ReadBufferOnceUpToLength(dst.size())}) {
+ uint64_t pos{(m_read_pos - bytes_read) % vchBuf.size()};
+ memcpy(dst.data(), &vchBuf[pos], bytes_read);
+ dst = dst.subspan(bytes_read);
}
}
@@ -697,11 +698,7 @@ public:
if (file_pos > nReadLimit) {
throw std::ios_base::failure("Attempt to position past buffer limit");
}
- while (m_read_pos < file_pos) {
- uint64_t pos{m_read_pos % vchBuf.size()};
- size_t length{FillBufferUpToLength(pos, file_pos - m_read_pos)};
- m_read_pos += length;
- }
+ while (ReadBufferOnceUpToLength(file_pos - m_read_pos));
}
//! return the current reading position
</details>
