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.

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

Panic when advance_slices()'ing too far and update docs. #94855

Merged
merged 3 commits into from
Jun 20, 2022
Merged

Panic when advance_slices()'ing too far and update docs. #94855

merged 3 commits into from
Jun 20, 2022

Conversation

m-ou-se
Copy link
Member

@m-ou-se m-ou-se commented Mar 11, 2022

This updates advance_slices() to panic when advancing too far, like advance() already does. And updates the docs to say so.

See #62726 (comment)

@m-ou-se m-ou-se added T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. A-io Area: `std::io`, `std::fs`, `std::net` and `std::path` labels Mar 11, 2022
@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 Mar 11, 2022
@m-ou-se m-ou-se mentioned this pull request Mar 11, 2022
Closed
3 tasks
Thomasdezeeuw
Thomasdezeeuw reviewed Mar 11, 2022
View reviewed changes
Copy link
Contributor

@Thomasdezeeuw Thomasdezeeuw left a comment

Choose a reason for hiding this comment

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

Don't really have a strong opinion here. On one hand it introduces an additional assert (don't know if it's compiled away), on the other hand having consistent behaviour with advance which is pretty nice.

@m-ou-se
Copy link
Member Author

m-ou-se commented Mar 11, 2022

The number of bytes to advance will in most cases be the result of a syscall, so in those cases the check can't be optimized away, unfortunately.

Consistency with advance seems important though. And panicking when things are out of bounds is the standard behaviour of basically everything in Rust.

kennytm
kennytm approved these changes Mar 12, 2022
View reviewed changes
library/std/src/io/mod.rs
@@ -1155,7 +1162,9 @@ impl<'a> IoSliceMut<'a> {
}

*bufs = &mut replace(bufs, &mut [])[remove..];
if !bufs.is_empty() {
if bufs.is_empty() {
assert!(n == accumulated_len, "advancing io slices beyond their length");
Copy link
Member

Choose a reason for hiding this comment

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

maybe use assert_eq here to include the expected length in the panic message?

Copy link
Member Author

Choose a reason for hiding this comment

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

Then the user would just see left: 200, right: 100, which doesn't mean much. (Also, assert_eq results in mode codegen, which is a bit of a waste.)

Copy link
Member

Choose a reason for hiding this comment

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

ok

@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 Apr 11, 2022
@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 May 8, 2022
@JohnCSimon
Copy link
Member

triage:
this already has an approval

@m-ou-se
Copy link
Member Author

m-ou-se commented Jun 20, 2022

@bors r=kennytm

@bors
Copy link
Contributor

bors commented Jun 20, 2022

📌 Commit 1890372 has been approved by kennytm

@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 Jun 20, 2022
@Dylan-DPC Dylan-DPC mentioned this pull request Jun 20, 2022
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 20, 2022
@bors
Auto merge of rust-lang#98292 - Dylan-DPC:rollup-hueb8tm, r=Dylan-DPC
b12708f 
Rollup of 8 pull requests

Successful merges:

 - rust-lang#93080 (Implement `core::slice::IterMut::as_mut_slice` and `impl<T> AsMut<[T]> for IterMut<'_, T>`)
 - rust-lang#94855 (Panic when advance_slices()'ing too far and update docs.)
 - rust-lang#96609 (Add `{Arc, Rc}::downcast_unchecked`)
 - rust-lang#96719 (Fix the generator example for `pin!()`)
 - rust-lang#97149 (Windows: `CommandExt::async_pipes`)
 - rust-lang#97150 (`Stdio::makes_pipe`)
 - rust-lang#97837 (Document Rust's stance on `/proc/self/mem`)
 - rust-lang#98159 (Include ForeignItem when visiting types for WF check)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 99620ad into rust-lang:master Jun 20, 2022
@rustbot rustbot added this to the 1.63.0 milestone Jun 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Thomasdezeeuw Thomasdezeeuw Thomasdezeeuw left review comments

@kennytm kennytm kennytm approved these changes

Assignees

@kennytm kennytm

Labels
A-io Area: `std::io`, `std::fs`, `std::net` and `std::path` 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
Milestone
1.63.0
Development

Successfully merging this pull request may close these issues.
7 participants
@m-ou-se @rust-highfive @JohnCSimon @bors @kennytm @Thomasdezeeuw @rustbot