Skip to content

Improve several Read implementations #122441

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
May 4, 2024
Merged

Improve several Read implementations #122441

merged 3 commits into from
May 4, 2024

Conversation

a1phyr
Copy link
Contributor

@a1phyr a1phyr commented Mar 13, 2024 •
edited
Loading

  • 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
zachs18
zachs18 reviewed Mar 14, 2024
View reviewed changes
Copy link
Contributor

@zachs18 zachs18 left a comment •
edited
Loading

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 🤷.

@a1phyr a1phyr force-pushed the improve_read_impls branch from cc92a9b to 0db18e0 Compare March 14, 2024 09:32
@a1phyr
Copy link
Contributor Author

a1phyr commented Mar 14, 2024 •
edited
Loading

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.

@a1phyr a1phyr force-pushed the improve_read_impls branch from 0db18e0 to df0d955 Compare March 14, 2024 10:10
@workingjubilee
Copy link
Member

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

@a1phyr a1phyr mentioned this pull request Mar 26, 2024
Merged
@a1phyr a1phyr force-pushed the improve_read_impls branch from 7e790d3 to 576a0ba Compare March 26, 2024 09:17
workingjubilee added a commit to workingjubilee/rustc that referenced this pull request Mar 26, 2024
@workingjubilee
Rollup merge of rust-lang#123084 - a1phyr:unixstream_read_buf, r=work…
9bebf31 
…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
@matthiaskrgr
Rollup merge of rust-lang#123084 - a1phyr:unixstream_read_buf, r=work…
910e23b 
…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
@rust-timer
Unrolled build for rust-lang#123084
edf466b 
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
Collaborator

bors commented Apr 10, 2024

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

@a1phyr a1phyr force-pushed the improve_read_impls branch from 576a0ba to d1d142b Compare April 10, 2024 17:17
@bors
Copy link
Collaborator

bors commented Apr 11, 2024

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

@a1phyr a1phyr force-pushed the improve_read_impls branch from d1d142b to 2e3ee23 Compare April 12, 2024 07:47
@ChrisDenton
Copy link
Member

lgtm. And thanks for @zachs18 for also reviewing!

@bors r+

@bors
Copy link
Collaborator

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
@matthiaskrgr
Rollup merge of rust-lang#122441 - a1phyr:improve_read_impls, r=Chris…
76dc302 
…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`
@matthiaskrgr matthiaskrgr mentioned this pull request May 4, 2024
Closed
bors added a commit to rust-lang-ci/rust that referenced this pull request May 4, 2024
@bors
Auto merge of rust-lang#124710 - matthiaskrgr:rollup-per6u61, r=matth…
7852822 
…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
@bors
Auto merge of rust-lang#124710 - matthiaskrgr:rollup-per6u61, r=matth…
8ea632f 
…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
@matthiaskrgr matthiaskrgr mentioned this pull request May 4, 2024
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request May 4, 2024
@bors
Auto merge of rust-lang#124716 - matthiaskrgr:rollup-ni58ie1, r=matth…
d568423 
…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
@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
@rust-timer
Unrolled build for rust-lang#122441
996721e 
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`
@thaliaarchi thaliaarchi mentioned this pull request Feb 15, 2025
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
1 more reviewer

@zachs18 zachs18 zachs18 left review comments

Reviewers whose approvals may not affect merge requirements
Assignees

@ChrisDenton ChrisDenton

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
Milestone
1.80.0
Development

Successfully merging this pull request may close these issues.
6 participants
@a1phyr @rustbot @workingjubilee @bors @ChrisDenton @zachs18