feat: add IO stream infrastructure#39
Conversation
leaves12138
left a comment
leaves12138
left a comment
There was a problem hiding this comment.
I found correctness blockers in the new stream implementations: failed seeks can leave stream positions corrupted, and OffsetInputStream boundary checks truncate 64-bit offsets. Please fix these before merging.
| Status ByteArrayInputStream::Seek(int64_t offset, SeekOrigin origin) { | ||
| switch (origin) { | ||
| case SeekOrigin::FS_SEEK_SET: { | ||
| position_ = offset; |
There was a problem hiding this comment.
Please compute the target position in a local variable and assign position_ only after validation succeeds. Currently a failed seek (for example, seeking past EOF) returns an error but leaves position_ out of range, so subsequent GetPos()/Read() calls observe corrupted stream state. The same invariant should hold for FS_SEEK_CUR and FS_SEEK_END.
| Status OffsetInputStream::Seek(int64_t offset, SeekOrigin origin) { | ||
| switch (origin) { | ||
| case SeekOrigin::FS_SEEK_SET: { | ||
| inner_position_ = offset; |
There was a problem hiding this comment.
Same issue here: inner_position_ is mutated before both boundary validation and the wrapped seek succeed. A failed seek should not change the logical position. Please calculate a local target position first, validate it, seek the wrapped stream, and only then commit inner_position_.
| return length_; | ||
| } | ||
|
|
||
| Status OffsetInputStream::AssertBoundary(int32_t inner_pos) const { |
There was a problem hiding this comment.
This boundary check should not take int32_t. The stream API and length_ use 64-bit offsets, so passing values like inner_position_ + size or offset + size through int32_t can truncate offsets above 2 GiB and either reject valid reads or allow out-of-range reads. Please use a 64-bit type consistently here.
leaves12138
left a comment
leaves12138
left a comment
There was a problem hiding this comment.
Re-reviewed PR #39. The remaining concerns about failed seek position rollback and int64_t file system offsets are planned for a later unified file-system refactor, so they should not block this PR. I did not find other blockers.
2 participants
Changes
DataInputStreamint8,int16,int32,int64,float,double) and byte arraysDataOutputStreamByteArrayInputStreamDataInputStreambacked by an in-memory byte array (Bytes), supporting sequential reads with position trackingBufferedInputStreamDataInputStreamwith configurable buffer size, supportingSeek(),Skip(), and efficient sequential readsOffsetInputStreamDataInputStreamthat reads from a sub-range of an underlying stream, defined by offset and length, with boundary enforcementMemorySegmentOutputStreamDataOutputStreambacked byMemorySegmentwith auto-growing capacity, producingMemorySliceoutput viaToSlice()Tests
BufferedInputStreamTestByteArrayInputStreamTestDataInputOutputStreamTestMemorySegmentOutputStreamTestOffsetInputStreamTest