Skip to content

Merge PeekStream and BufferedInStream #4878

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Conversation

daurnimator
Copy link
Contributor

Child of #4505
Closes #4501

@daurnimator daurnimator added the standard library This issue involves writing Zig code for the standard library. label Mar 31, 2020
@joachimschmidt557
Copy link
Contributor

I'm very unsure about this, but: Isn't the behaviour of PeekStream.read different from BufferedInStream.read? A PeekStream.read never fills the FIFO, only PeekStream.putBack can do that. But a BufferedInStream does automatically add to the FIFO while reading to speed things up.

Couldn't that create a problem when I try to putBack something and it doesn't work with the merged API because read already filled the FIFO to its limit e.g. when I have a lookahead of exactly 1 byte?

@daurnimator daurnimator force-pushed the merge-peekstream-and-BufferedInStreamCustom branch from 956243d to b1ff0a2 Compare April 3, 2020 10:27
@daurnimator
Copy link
Contributor Author

I'm very unsure about this, but: Isn't the behaviour of PeekStream.read different from BufferedInStream.read? A PeekStream.read never fills the FIFO, only PeekStream.putBack can do that. But a BufferedInStream does automatically add to the FIFO while reading to speed things up.

Yes.

Couldn't that create a problem when I try to putBack something and it doesn't work with the merged API because read already filled the FIFO to its limit e.g. when I have a lookahead of exactly 1 byte?

Ah, prior experience has made me take something for granted here: in the traditional libc/POSIX ungetc api, you are only allowed to put back what you last read: a subsequent read makes it undefined if you can fit things in or not. I did one-better than the libc API by at least allowing putBack to return an error if it doesn't fit: additionally, if you use a Dynamic buffer, then it will attempt to grow.

@daurnimator daurnimator force-pushed the merge-peekstream-and-BufferedInStreamCustom branch from b1ff0a2 to 38479bc Compare April 4, 2020 01:16
Copy link
Member

@andrewrk andrewrk left a comment

Choose a reason for hiding this comment

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

Can you update this to take into account the use case of being able to do a "put back" operation that is guaranteed to succeed? From the issue that this is trying to solve:

... guaranteed to be able to "put back" this many elements.

@andrewrk
Copy link
Member

Feel free to re-open with feedback addressed & rebased. It also sounds like it may be worth discussing on #4501 before implementing, since this takes the accepted proposal in a different direction.

@andrewrk andrewrk closed this Jun 25, 2020
@notcancername notcancername mentioned this pull request Dec 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
standard library This issue involves writing Zig code for the standard library.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

provide a peeking API to std.io.BufferedReader
3 participants