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

Improve several Read implementations #122441

Merged
merged 3 commits into from
May 4, 2024
Merged

Conversation

a1phyr
Copy link
Contributor

@a1phyr a1phyr commented Mar 13, 2024

  • read_to_end and read_to_string for Cursor
  • Error on OOM in read_to_string of &[u8] and VecDeque<u8>
  • Avoid making the slices contiguous in VecDeque::read_to_string
  • read_exact and (unstable) read_buf_exact for Take
  • read_buf for UnixStream and &UnixStream (moved to UnixStream: override read_buf #123084)
  • read_to_end for ChildStdErr

@rustbot
Copy link
Collaborator

rustbot commented Mar 13, 2024

r? @ChrisDenton

rustbot has assigned @ChrisDenton.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added O-unix Operating system: Unix-like S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Mar 13, 2024
Copy link
Contributor

@zachs18 zachs18 left a comment

Choose a reason for hiding this comment

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

Unrelated to code nits: Since Take::read_(buf_)exact don't actually call into the inner reader if buf.len() > self.limit, then you can get the weird case where take.read_exact(&mut long_buffer) returns EOF, but then immediately afterwards take.read_exact(&mut short_buffer) succeeds.

Perhaps self.limit should be set to 0 if Err(EOF) is returned from Take::read_(buf_)exact? Though this would "decouple" the limit from how many bytes are actually read, but that already happens if we do call into the inner reader and its read_exact fails, so 🤷.

library/std/src/io/mod.rs Outdated Show resolved Hide resolved
library/std/src/io/mod.rs Outdated Show resolved Hide resolved
library/std/src/io/mod.rs Outdated Show resolved Hide resolved
library/std/src/io/mod.rs Outdated Show resolved Hide resolved
@a1phyr
Copy link
Contributor Author

a1phyr commented Mar 14, 2024

Perhaps self.limit should be set to 0 if Err(EOF) is returned from Take::read_(buf_)exact? Though this would "decouple" the limit from how many bytes are actually read, but that already happens if we do call into the inner reader and its read_exact fails, so 🤷.

I think that Take should never read more that limit bytes, whatever the circumstances, and decoupling seems wrong to me too. Only the default implementation feels correct, so I have removed Take::read_(buf_)exact from the PR.

One thing though, &[u8] and Cursor have this problem, so this should probably be changed in a follow-up PR.

@workingjubilee
Copy link
Member

@a1phyr Can you split out the Unix-specific changes into their own PR?

workingjubilee added a commit to workingjubilee/rustc that referenced this pull request Mar 26, 2024
…ingjubilee

`UnixStream`: override `read_buf`

Split from rust-lang#122441

r? `@workingjubilee`
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Mar 27, 2024
…ingjubilee

`UnixStream`: override `read_buf`

Split from rust-lang#122441

r? ``@workingjubilee``
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Mar 27, 2024
Rollup merge of rust-lang#123084 - a1phyr:unixstream_read_buf, r=workingjubilee

`UnixStream`: override `read_buf`

Split from rust-lang#122441

r? ``@workingjubilee``
@bors
Copy link
Contributor

bors commented Apr 10, 2024

☔ The latest upstream changes (presumably #122393) made this pull request unmergeable. Please resolve the merge conflicts.

@bors
Copy link
Contributor

bors commented Apr 11, 2024

☔ The latest upstream changes (presumably #123732) made this pull request unmergeable. Please resolve the merge conflicts.

@ChrisDenton
Copy link
Member

lgtm. And thanks for @zachs18 for also reviewing!

@bors r+

@bors
Copy link
Contributor

bors commented May 4, 2024

📌 Commit 2e3ee23 has been approved by ChrisDenton

It is now in the queue for this repository.

@bors bors added 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-review Status: Awaiting review from the assignee but also interested parties. labels May 4, 2024
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request May 4, 2024
…Denton

Improve several `Read` implementations

- `read_to_end` and `read_to_string` for `Cursor`
- Error on OOM in `read_to_string` of `&[u8]` and `VecDeque<u8>`
- Avoid making the slices contiguous in `VecDeque::read_to_string`
- ~`read_exact` and (unstable) `read_buf_exact` for `Take`~
- ~`read_buf` for `UnixStream` and `&UnixStream`~ (moved to rust-lang#123084)
- `read_to_end` for `ChildStdErr`
bors added a commit to rust-lang-ci/rust that referenced this pull request May 4, 2024
…iaskrgr

Rollup of 5 pull requests

Successful merges:

 - rust-lang#122441 (Improve several `Read` implementations)
 - rust-lang#124584 (Various improvements to entrypoint code)
 - rust-lang#124699 (Use `unchecked_sub` in `split_at`)
 - rust-lang#124704 (Fix ignored tests for formatting)
 - rust-lang#124709 (Miri subtree update)

r? `@ghost`
`@rustbot` modify labels: rollup
bors added a commit to rust-lang-ci/rust that referenced this pull request May 4, 2024
…iaskrgr

Rollup of 5 pull requests

Successful merges:

 - rust-lang#122441 (Improve several `Read` implementations)
 - rust-lang#124584 (Various improvements to entrypoint code)
 - rust-lang#124699 (Use `unchecked_sub` in `split_at`)
 - rust-lang#124704 (Fix ignored tests for formatting)
 - rust-lang#124709 (Miri subtree update)

r? `@ghost`
`@rustbot` modify labels: rollup
bors added a commit to rust-lang-ci/rust that referenced this pull request May 4, 2024
…iaskrgr

Rollup of 4 pull requests

Successful merges:

 - rust-lang#122441 (Improve several `Read` implementations)
 - rust-lang#124584 (Various improvements to entrypoint code)
 - rust-lang#124699 (Use `unchecked_sub` in `split_at`)
 - rust-lang#124715 (interpret, miri: uniform treatments of intrinsics/functions with and without return block)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 00bc2a4 into rust-lang:master May 4, 2024
11 checks passed
@rustbot rustbot added this to the 1.80.0 milestone May 4, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request May 4, 2024
Rollup merge of rust-lang#122441 - a1phyr:improve_read_impls, r=ChrisDenton

Improve several `Read` implementations

- `read_to_end` and `read_to_string` for `Cursor`
- Error on OOM in `read_to_string` of `&[u8]` and `VecDeque<u8>`
- Avoid making the slices contiguous in `VecDeque::read_to_string`
- ~`read_exact` and (unstable) `read_buf_exact` for `Take`~
- ~`read_buf` for `UnixStream` and `&UnixStream`~ (moved to rust-lang#123084)
- `read_to_end` for `ChildStdErr`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
O-unix Operating system: Unix-like S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants