Skip to content

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

Merged
bors merged 3 commits intorust-lang:masterfrom
m-ou-se:advance-slice-panic-docs
Jun 20, 2022
Merged

Panic when advance_slices()'ing too far and update docs.#94855
bors merged 3 commits intorust-lang:masterfrom
m-ou-se:advance-slice-panic-docs

Conversation

@m-ou-se
Copy link
Copy Markdown
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
Copy Markdown
Contributor

rust-highfive commented Mar 11, 2022

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
@m-ou-se m-ou-se force-pushed the advance-slice-panic-docs branch from 6a4862e to 4d7daa0 Compare March 11, 2022 16:30
Thomasdezeeuw
Thomasdezeeuw reviewed Mar 11, 2022
View reviewed changes
Copy link
Copy Markdown
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
Copy Markdown
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
*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
Copy Markdown
Member

@kennytm kennytm Mar 12, 2022

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
Copy Markdown
Member Author

@m-ou-se m-ou-se Mar 12, 2022

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
Copy Markdown
Member

@kennytm kennytm Mar 12, 2022

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
Copy Markdown
Member

JohnCSimon commented Jun 20, 2022

triage:
this already has an approval

@m-ou-se
Copy link
Copy Markdown
Member Author

m-ou-se commented Jun 20, 2022

@bors r=kennytm

@bors
Copy link
Copy Markdown
Collaborator

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

@kennytm kennytm kennytm approved these changes

+1 more reviewer

@Thomasdezeeuw Thomasdezeeuw Thomasdezeeuw left review comments

Reviewers whose approvals may not affect merge requirements

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