Add buffered line reads to BufferedSource#14845
Conversation
|
Any chance we could use Pipe class in tests? |
|
Also could we test this code with different buffer sizes? To catch various corner-cases. Maybe make a parametric test fixture and feed random data (mt19937 maybe?) into the source and test all against an unbuffered version via a StringSource (or does that not take a Source &? In that case we can just di a temp file). This buffer handling is pretty hard, so I want us to get it 100% right |
Zaczero
force-pushed
the
zaczero/BufferedSource--readLine
branch
2 times, most recently
from
98b425c to
185bbe0
Compare
|
I added buffer oriented tests |
src/libutil-tests/file-descriptor.cc
Outdated
|
|
||
| #include <cstring> | ||
|
|
||
| #ifndef _WIN32 |
There was a problem hiding this comment.
Generally the ifdef should be redundant, since there's a windows-specific implemntation of Pipe (not sure how well tested it is).
src/libutil/serialise.cc
Outdated
| std::string BufferedSource::readLine(bool eofOk) | ||
| { | ||
| if (!buffer) | ||
| buffer = decltype(buffer)(new char[bufSize]); |
There was a problem hiding this comment.
We could probably use this, but that's just stylistic nitpick and pre-existing, so just a nice-to-have.
| buffer = decltype(buffer)(new char[bufSize]); | |
| buffer = std::make_unique_for_overwrite<char[]>(bufSize); |
| auto * start = buffer.get() + bufPosOut; | ||
| auto * end = buffer.get() + bufPosIn; | ||
| if (auto * newline = static_cast<char *>(memchr(start, '\n', end - start))) { |
There was a problem hiding this comment.
Hope we could wrap this in a helper method that returns a std::span. This manual handling of bounds is quite tricky.
| bufPosOut = bufPosIn = 0; | ||
| if (eofOk) | ||
| return line; | ||
| throw EndOfFile("unexpected EOF reading a line"); |
There was a problem hiding this comment.
This and below could maybe get deduplicated with a lambda, but that's short enough to maybe not warrant that.
Provide BufferedSource::readLine for opt-in buffered line reading. Migrate applicable call sites.
xokdvium
force-pushed
the
zaczero/BufferedSource--readLine
branch
from
185bbe0 to
b813ed2
Compare
|
Fixed those minor nitpicks that I had. |
2 participants
Provide BufferedSource::readLine for opt-in buffered line reading. Migrate applicable call sites.
Succeeds #14829
Motivation
Context
Add 👍 to pull requests you find important.
The Nix maintainer team uses a GitHub project board to schedule and track reviews.