-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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
Document unsafety in slice/sort.rs #71568
Conversation
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @joshtriplett (or someone else) soon. If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes. Please see the contribution instructions for more information. |
@rustbot modify labels to +S-waiting-review -S-waiting-on-author |
This is just the reverse of shift_head.
We already implicitly (or explicitly??) do the bound checking for the indexing.
These are simply indexing safety.
src/libcore/slice/sort.rs
Outdated
// 1. We are obtaining pointers to references which are guaranteed to be valid. | ||
// 2. They cannot overlap because we obtain pointers to difference indices of the slice. | ||
// Namely, `i` and `i-1`. | ||
// 3. FIXME: Guarantees that the elements are properly aligned? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the slice is properly aligned, the elements will be properly aligned. And it's the caller's responsibility to make sure the slice is properly aligned.
src/libcore/slice/sort.rs
Outdated
// 1. We are obtaining pointers to references which are guaranteed to be valid. | ||
// 2. They cannot overlap because we obtain pointers to difference indices of the slice. | ||
// Namely, `i` and `i+1`. | ||
// 3. FIXME: Guarantees that the elements are properly aligned? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comment as above.
src/libcore/slice/sort.rs
Outdated
@@ -103,6 +131,8 @@ where | |||
let mut i = 1; | |||
|
|||
for _ in 0..MAX_STEPS { | |||
// SAFETY: We already explicitly done the bound checking with `i<len` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// SAFETY: We already explicitly done the bound checking with `i<len` | |
// SAFETY: We already explicitly did the bound checking with `i<len` |
src/libcore/slice/sort.rs
Outdated
@@ -103,6 +131,8 @@ where | |||
let mut i = 1; | |||
|
|||
for _ in 0..MAX_STEPS { | |||
// SAFETY: We already explicitly done the bound checking with `i<len` | |||
// All our indexing following that is only in the range {0 <= index < len} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// All our indexing following that is only in the range {0 <= index < len} | |
// All our subsequent indexing is only in the range `0 <= index < len` |
src/libcore/slice/sort.rs
Outdated
// | ||
// a. Indexing: | ||
// 1. We checked the size of the array to >=2. | ||
// 2. All the indexing that we will do is always between {0 <= index < len-1} at most. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// 2. All the indexing that we will do is always between {0 <= index < len-1} at most. | |
// 2. All the indexing that we will do is always between `0 <= index < len-1` at most. |
src/libcore/slice/sort.rs
Outdated
@@ -404,8 +435,12 @@ where | |||
// Find the first pair of out-of-order elements. | |||
let mut l = 0; | |||
let mut r = v.len(); | |||
|
|||
// SAFETY: The unsafety below involves indexing an array. | |||
// For the first one: we already do the bound checking here with `l<r`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// For the first one: we already do the bound checking here with `l<r`. | |
// For the first one: we already do the bounds checking here with `l < r`. |
src/libcore/slice/sort.rs
Outdated
|
||
// SAFETY: The unsafety below involves indexing an array. | ||
// For the first one: we already do the bound checking here with `l<r`. | ||
// For the secondn one: the minimum value for `l` is 0 and the maximum value for `r` is `v.len().` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// For the secondn one: the minimum value for `l` is 0 and the maximum value for `r` is `v.len().` | |
// For the second one: the minimum value for `l` is 0 and the maximum value for `r` is `v.len().` |
src/libcore/slice/sort.rs
Outdated
@@ -452,8 +488,11 @@ where | |||
let mut l = 0; | |||
let mut r = v.len(); | |||
loop { | |||
// SAFETY: The unsafety below involves indexing an array. | |||
// For the first one: we already do the bound checking here with `l<r`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// For the first one: we already do the bound checking here with `l<r`. | |
// For the first one: we already do the bounds checking here with `l < r`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I posted a few review comments; with those fixed, r=me.
Thanks @joshtriplett ! I have applied your requested changes and added some more safety comments. I read through the discussions by more senior people over at #63793 |
919aa86
to
31c8674
Compare
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
Looks good to me (and definitely not too verbose). However, it looks like the build is failing because you removed |
31c8674
to
8687dec
Compare
src/libcore/slice/sort.rs
Outdated
use crate::cmp; | ||
use crate::mem::{self, MaybeUninit}; | ||
use crate::ptr; | ||
|
||
// ignore-tidy-undocumented-unsafe |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please don't move this comment below the use
lines like this. See the overall diff; these two lines shouldn't change at all.
8687dec
to
5a9df84
Compare
@bors r+ rollup |
📌 Commit 5a9df84 has been approved by |
…arth Rollup of 13 pull requests Successful merges: - rust-lang#71568 (Document unsafety in slice/sort.rs) - rust-lang#72709 (`#[deny(unsafe_op_in_unsafe_fn)]` in liballoc) - rust-lang#73214 (Add asm!() support for hexagon) - rust-lang#73248 (save_analysis: improve handling of enum struct variant) - rust-lang#73257 (ty: projections in `transparent_newtype_field`) - rust-lang#73261 (Suggest `?Sized` when applicable for ADTs) - rust-lang#73300 (Implement crate-level-only lints checking.) - rust-lang#73334 (Note numeric literals that can never fit in an expected type) - rust-lang#73357 (Use `LocalDefId` for import IDs in trait map) - rust-lang#73364 (asm: Allow multiple template string arguments; interpret them as newline-separated) - rust-lang#73382 (Only display other method receiver candidates if they actually apply) - rust-lang#73465 (Add specialization of `ToString for char`) - rust-lang#73489 (Refactor hir::Place) Failed merges: r? @ghost
Let me know if these documentations are accurate c:
I don't think I am capable enough to document the safety of
partition_blocks
, however.Related issue #66219