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

Implement most of RFC 2930, providing the ReadBuf abstraction #81156

Merged
merged 12 commits into from
Dec 9, 2021

Conversation

beepster4096
Copy link
Contributor

@beepster4096 beepster4096 commented Jan 18, 2021

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.

@rust-highfive
Copy link
Collaborator

r? @kennytm

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

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 18, 2021
@rust-log-analyzer

This comment has been minimized.

@jyn514
Copy link
Member

jyn514 commented Jan 18, 2021

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

@beepster4096
Copy link
Contributor Author

Alright I squashed it.

@rust-log-analyzer

This comment has been minimized.

@beepster4096
Copy link
Contributor Author

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

@rust-log-analyzer

This comment has been minimized.

@beepster4096
Copy link
Contributor Author

uh I think this PR is cursed

@rust-log-analyzer

This comment has been minimized.

@jyn514
Copy link
Member

jyn514 commented Jan 19, 2021

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.

@beepster4096
Copy link
Contributor Author

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

@djc
Copy link
Contributor

djc commented Jan 20, 2021

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!).

@bors

This comment has been minimized.

@JohnCSimon JohnCSimon added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 23, 2021
@bors

This comment has been minimized.

@bors

This comment has been minimized.

@kennytm
Copy link
Member

kennytm commented Mar 13, 2021

r? @KodrAus

@rust-highfive rust-highfive assigned KodrAus and unassigned kennytm Mar 13, 2021
@bstrie
Copy link
Contributor

bstrie commented Mar 31, 2021

r? @sfackler

@joshtriplett
Copy link
Member

joshtriplett commented Dec 8, 2021

In the interests of collecting possible naming considerations in one place:

  • The RFC uses assume_init.
  • assume_init seems evocative of an unsafe API: you're not making something initialized, you're making an assumption.
  • add_init or similar would more clearly indicate the addition to the initialized region, rather than replacing it.
  • Something like assume_added_init or add_assumed_init would have the benefits of both, but be more verbose.

@nrc
Copy link
Member

nrc commented Dec 8, 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?

@joshtriplett
Copy link
Member

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+

@rfcbot
Copy link

rfcbot commented Dec 8, 2021

@joshtriplett proposal cancelled.

@bors
Copy link
Contributor

bors commented Dec 8, 2021

📌 Commit cd23799 has been approved by joshtriplett

@rfcbot rfcbot removed proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels Dec 8, 2021
@bors 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 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
@Mark-Simulacrum
Copy link
Member

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
Copy link
Contributor

bors commented Dec 9, 2021

⌛ Testing commit cd23799 with merge 3b263ce...

@bors
Copy link
Contributor

bors commented Dec 9, 2021

☀️ Test successful - checks-actions
Approved by: joshtriplett
Pushing 3b263ce to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Dec 9, 2021
@bors bors merged commit 3b263ce into rust-lang:master Dec 9, 2021
@rustbot rustbot added this to the 1.59.0 milestone Dec 9, 2021
@bors bors mentioned this pull request Dec 9, 2021
9 tasks
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (3b263ce): comparison url.

Summary: This benchmark run did not return any relevant changes.

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

@rustbot label: -perf-regression

@rustbot rustbot removed the perf-regression Performance regression. label Dec 9, 2021
//
// - Only the standard library gets to soundly "ignore" this,
// based on its privileged knowledge of unstable rustc
// internals;
Copy link
Member

Choose a reason for hiding this comment

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

Ah, I am so happy to see this comment disappear. :)

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Dec 22, 2021
…yaahc

kmc-solid: Add `std::sys::solid::fs::File::read_buf`

This PR adds `std::sys::solid::fs::File::read_buf` to catch up with the changes introduced by rust-lang#81156 and fix the [`*-kmc-solid_*`](https://doc.rust-lang.org/nightly/rustc/platform-support/kmc-solid.html) Tier 3 targets..
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Tracking issue for Read::initializer