Implement most of RFC 2930, providing the ReadBuf abstraction by beepster4096 · Pull Request #81156 · rust-lang/rust (original) (raw)

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 andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation107 Commits12 Checks0 Files changed

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

This replaces the Initializer abstraction for permitting reading into uninitialized buffers, closing #42788.

This leaves several APIs described in the RFC out of scope for the initial implementation:

read_buf_vectored
ReadBufs

Closes #42788, by removing the relevant APIs.

fee1-dead, gilescope, andylizi, goffrie, paolobarbolini, Kobzol, gustav3d, and Yanhao reacted with thumbs up emoji hkratz, Kobzol, RalfJung, gilescope, and scottlamb reacted with hooray emoji iago-lito and gilescope reacted with heart emoji

r? @kennytm

(rust-highfive has picked a reviewer for you, use r? to override)

This comment has been minimized.

@drmeepster do you mind squashing the commits? 41 is a lot to read through.

This comment has been minimized.

Hopefully, weird errors in files I didn't touch don't show up this time.

This comment has been minimized.

uh I think this PR is cursed

This comment has been minimized.

uh I think this PR is cursed

We debugged this locally, here's an MCVE:

> cat doctest.rs
//! ```
//! extern crate proc_macro;
//!
//! # fn main() {}
//! ```
> rustdoc +rustc2 --test doctest.rs 

running 1 test
test doctest.rs - (line 1) ... FAILED

failures:

---- doctest.rs - (line 1) stdout ----
error: expected one of `;` or `as`, found `<eof>`
 --> doctest.rs:2:14
  |
2 | extern crate p
  |              ^ expected one of `;` or `as`

Notice how the error unexpectedly cuts off after crate p. So it's at least feasible it's related to this PR - either Rustdoc has UB or one of the changes introduced a logic error. It's strange that this only happens for rustdoc and not rustc.

The remove unresolved import commit passing CI means that the error is caused by the changes to BufReader

Very excited, thanks for working on this! I don't have a lot of experience with larger std changes like this, but I feel like it would be easier to land this if you focused on getting an initial PR merged that only includes the ReadBuf implementation (plus tests), and leave usage of the new type to future PRs (can still keep your changes, of course!).

This comment has been minimized.

anp mentioned this pull request

Apr 7, 2021

As far as I can tell, parts of this are insta-stable, and thus need an FCP

@joshtriplett which parts will be insta-stable?

We reviewed this in today's @rust-lang/libs-api meeting, and confirmed that there are no insta-stable bits of this. We do see two potential issues with this that should be covered and addressed in the tracking issue:

assume_init naming
&mut ReadBuf allows replacing the whole ReadBuf, which would allow pointing it to a different buffer of memory. Some callers may not expect that, and that may be error-prone for some high-performance use cases. We should consider having an opaque type wrapping a &mut ReadBuf (name subject to bikeshedding), adding a method to ReadBuf to return one, and then using that as the argument to read_buf.

However, since this isn't insta-stable, we can address both of those through further development on nightly.

@rfcbot cancel

@bors r+

📌 Commit cd23799 has been approved by joshtriplett

bors added disposition-merge

This issue / PR is in PFCP or FCP with a disposition to merge it.

S-waiting-on-bors

Status: Waiting on bors to run and complete tests. Bors will change the label on completion.

and removed S-waiting-on-author

Status: This is awaiting some action (such as code changes or more information) from the author.

labels

Dec 8, 2021

Mark-Simulacrum changed the title~~Implement (most of) RFC 2930 "Reading into uninitialized buffers" (ReadBuf)~~ Implement most of RFC 2930, providing the ReadBuf abstraction

Dec 9, 2021

FWIW, I think it would be nice to squash the commits here, but it doesn't seem worth blocking over and it looks like the feature letting upstream maintainers push to the branch isn't turned on for this PR.

bors mentioned this pull request

Dec 9, 2021

9 tasks

nrc mentioned this pull request