Skip to content

Conversation

@scorphus
Copy link
Contributor

@scorphus scorphus commented Jan 9, 2022

This is one way I see #9594 being solved. I hope I didn't get it too far off. I'm more than happy making any changes (and learn in the process)

I decided to start with readUntilDelimiter and readUntilDelimiterOrEof and submit this as draft to collect early feedbacks — which are more than welcome 🙂 I'll continue to the other readUntilDelimiter* functions.

Happy to propose my first contribution to Zig! 🎉 Thanks to @SpexGuy for suggesting me this issue.

@SpexGuy
Copy link
Contributor

SpexGuy commented Jan 9, 2022

Not all readers support seekBy. I think a better solution would be to avoid reading at all if the output buffer is full.

@scorphus
Copy link
Contributor Author

scorphus commented Jan 9, 2022

Not all readers support seekBy. I think a better solution would be to avoid reading at all if the output buffer is full.

Yeah, it's so true that most streams don't support rewinding. #mybad

I actually thought about that a few iterations before. Without reading the next byte, how's it going to be able to stop and return what can be a full buffer?

@scorphus
Copy link
Contributor Author

scorphus commented Jan 9, 2022

For instance:

test "Reader.readUntilDelimiter EndOfStream 2" {
    var buf: [4]u8 = undefined;
    const reader = std.io.fixedBufferStream("1234\n").reader();
    try std.testing.expectEqualStrings("1234", try reader.readUntilDelimiter(&buf, '\n'));
    try std.testing.expectError(error.EndOfStream, reader.readUntilDelimiter(&buf, '\n'));
}

@scorphus scorphus closed this Jan 9, 2022
@scorphus scorphus reopened this Jan 9, 2022
@SpexGuy
Copy link
Contributor

SpexGuy commented Jan 9, 2022

That test would no longer pass, the buffer would need to be at least 5 bytes long. An easy way to communicate that might be to say that the output buffer needs to contain (or be terminated by) the delimiter if one was found.

@scorphus scorphus force-pushed the 9594-readUntilDelimiter branch from 89f5828 to d789552 Compare January 9, 2022 20:46
@scorphus
Copy link
Contributor Author

scorphus commented Jan 9, 2022

Thanks, @SpexGuy! PR updated. I would appreciate reading from you again, while I write tests for the remaining readUntilDelimiter* and start checking namespaces that use them (e.g. std.net).

Copy link
Contributor

@SpexGuy SpexGuy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for doing this work!

@scorphus scorphus force-pushed the 9594-readUntilDelimiter branch from d789552 to 0e09fe1 Compare January 10, 2022 21:53
@scorphus
Copy link
Contributor Author

You're welcome. I'm happy to contribute 🙂 And this is fun! It makes much sense after I thought about what you wrote. Thanks, @SpexGuy!

Copy link
Contributor

@SpexGuy SpexGuy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just noticed one more thing :)

/// large enough to hold the entire contents, `error.StreamTooLong` is returned.
/// Returns a slice of the stream data, with ptr equal to `buf.ptr`. The
/// delimiter byte is not included in the returned slice.
pub fn readUntilDelimiter(self: Self, buf: []u8, delimiter: u8) ![]u8 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The documentation should be updated to say something like "The delimiter byte is written to the output buffer, but is not included in the returned slice."

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh yes, definitely! I'm only waiting until we have a resolute "new behavior" so I can update all those docs ;-)

@scorphus scorphus force-pushed the 9594-readUntilDelimiter branch 2 times, most recently from 57f001f to bcae596 Compare January 15, 2022 23:10
@scorphus scorphus marked this pull request as ready for review January 15, 2022 23:10
@scorphus scorphus changed the title std.io.Reader.readUntilDelimiter*: rewind 1 position if past max size readUntilDelimiter*: read only if buffer not full Jan 16, 2022
@scorphus scorphus requested a review from SpexGuy January 19, 2022 09:31
@Vexu Vexu merged commit 5ba4385 into ziglang:master Jan 24, 2022
@scorphus scorphus deleted the 9594-readUntilDelimiter branch January 24, 2022 19:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants