Skip to content
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

Add read_into_buf and get_buf to BufRead #1015

Closed
wants to merge 1 commit into from

Conversation

seanmonstar
Copy link
Contributor

Add two methods to BufRead, read_into_buf and get_buf. These would allow filling up more of the buffer, and inspecting the buffered data without causing an implicit read.

Rendered

cc @aturon @alexcrichton @sfackler

@oli-obk
Copy link
Contributor

oli-obk commented Apr 7, 2015

I always assumed fill_buf to act as your read_into_buf + get_buf together. I guess I was lucky that this didn't bite me yet.

@seanmonstar
Copy link
Contributor Author

An implementation is currently used in hyper. It was unfortunate needing to copy the BufReader from std, since otherwise the buf field is private and these methods couldn't be implemented as a BufExt trait or whichever.

@aturon aturon self-assigned this Apr 9, 2015
@erickt
Copy link

erickt commented Apr 13, 2015

This is a particularly useful pattern for high performance parsing. For example, one of the tricks rapidjson does to speed up parsing is to use SIMD instructions to parse N whitespace characters at a time. We could use this trick if we could get access to the underlying BufRead buffer (and of course my long-desired IntoBufferedRead trait 😄).

@seanmonstar
Copy link
Contributor Author

@erickt that's pretty much my exact usecase (except parsing HTTP instead of JSON).

@alexcrichton alexcrichton added the T-libs-api Relevant to the library API team, which will review and decide on the RFC. label May 18, 2015
@BurntSushi
Copy link
Member

I think this RFC as written proposes a backwards incompatible change, yes? Existing implementations of BufRead would break with the addition of non-default methods.

Is there a way to salvage the RFC? (I don't see any obvious way to provide default impls of these methods.)

@seanmonstar
Copy link
Contributor Author

I don't believe a default could be provided for these methods. To salvage this, it would need... to be a new trait? trait BufReadExt: BufRead {}?


/// This will read from the inner Read, and append the data onto the end of
/// any currently buffered data.
fn read_into_buf(&mut self) -> io::Result<usize>;
Copy link
Member

Choose a reason for hiding this comment

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

The return value is... how much data is currently in the buffer, or how much was read by just this call?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It returns how much was read by this call. You can check the size of the current buffer with get_buf().len().

@alexcrichton
Copy link
Member

This RFC is now entering its week-long final comment period.

@alexcrichton alexcrichton added the final-comment-period Will be merged/postponed/closed in ~10 calendar days unless new substational objections are raised. label Jun 16, 2015
@alexcrichton
Copy link
Member

I agree with @seanmonstar's analysis that this can no longer be implemented (due to stabilization), and it doesn't seem worth it to add an extra trait at this time.

@seanmonstar
Copy link
Contributor Author

Something I thought of also, would be to provide default implementations with unimplemented!(). If there was a way to add a warning for implementors that they should override the default, then it will prevent breakage while poking people to implement it.

This solution would allow adding methods to any trait.

@rkjnsn
Copy link
Contributor

rkjnsn commented Jun 19, 2015

I've wanted this exact functionality myself, so 👍 to the idea. It'll be nice if we can find a way to add this functionality in a backward-compatible way, but if not, I think this should be a strong contender for adding in 2.0.

@alexcrichton
Copy link
Member

The consensus of the libs team was to close this RFC at this time. We think that extension traits should bake outside the standard library first, and otherwise it looks like this RFC may want to start anew with a fresh look at backwards-compatibility. Thanks regardless for the RFC @seanmonstar!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
final-comment-period Will be merged/postponed/closed in ~10 calendar days unless new substational objections are raised. T-libs-api Relevant to the library API team, which will review and decide on the RFC.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants