Panic when advance_slices()'ing too far and update docs. by m-ou-se · Pull Request #94855 · rust-lang/rust (original) (raw)
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 andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation9 Commits3 Checks0 Files changed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
This updates advance_slices() to panic when advancing too far, like advance() already does. And updates the docs to say so.
See #62726 (comment)
Relevant to the library API team, which will review and decide on the PR/issue.
Area: `std::io`, `std::fs`, `std::net` and `std::path`
labels
r? @kennytm
(rust-highfive has picked a reviewer for you, use r? to override)
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.
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.
| @@ -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"); |
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?
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.)
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok
triage:
this already has an approval
📌 Commit 1890372 has been approved by kennytm
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
bors added a commit to rust-lang-ci/rust that referenced this pull request
Rollup of 8 pull requests
Successful merges:
- rust-lang#93080 (Implement
core::slice::IterMut::as_mut_sliceandimpl<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
Labels
Area: `std::io`, `std::fs`, `std::net` and `std::path`
Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Relevant to the library API team, which will review and decide on the PR/issue.