Document unsafety in src/libcore/slice/mod.rs#73555
Closed
LeSeulArtichaut wants to merge 1 commit intorust-lang:masterfrom
Closed
Document unsafety in src/libcore/slice/mod.rs#73555LeSeulArtichaut wants to merge 1 commit intorust-lang:masterfrom
src/libcore/slice/mod.rs#73555LeSeulArtichaut wants to merge 1 commit intorust-lang:masterfrom
Conversation
rust-highfive
added
the
S-waiting-on-review
Contributor
|
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 |
LeSeulArtichaut
force-pushed
the
document-unsafety
branch
from
1930b7a to
bcd9688
Compare
Collaborator
|
☔ The latest upstream changes (presumably #73643) made this pull request unmergeable. Please resolve the merge conflicts. |
LeSeulArtichaut
force-pushed
the
document-unsafety
branch
from
bcd9688 to
a4095ca
Compare
Contributor
Author
|
Rebased to resolve the conflicts. |
| // SAFETY: the condition of the `while` guarantees that | ||
| // `i` and `ln - i - chunk` are inside the slice. | ||
| // The resulting pointers `pa` and `pb` are therefore valid, | ||
| // and can be read from and written to. |
Member
There was a problem hiding this comment.
These comments also need to touch on the larger-pieced bytes being still in-bounds based on the indexing. Personally I would like to see some short proof-y wording, maybe something like this.
Personally I feel like just saying "well the condition guarantees things" doesn't help much since it doesn't allow for modifying or understanding why this is the case.
An unaligned u32 can be read from i if i + 1 < ln (and obviously i < ln),
because each element is 2 bytes and we're reading 4.
i + chunk - 1 < ln / 2 # while condition
i + 2 - 1 < ln / 2
i + 1 < ln / 2
Since it's less than the length divided by 2, then it must be in bounds.
| let half = size / 2; | ||
| let mid = base + half; | ||
| // mid is always in [0, size), that means mid is >= 0 and < size. | ||
| // SAFETY: mid is always in [0, size), that means mid is >= 0 and < size. |
Member
There was a problem hiding this comment.
Suggested change
| // SAFETY: mid is always in [0, size), that means mid is >= 0 and < size. | |
| // SAFETY: |
Obviously pre-existing but this is not really saying anything and the next two lines are precisely what we need
| let mut next_write: usize = 1; | ||
|
|
||
| // SAFETY: the `while` condition guarantees `next_read` and `next_write` | ||
| // are less than `len`, thus are inside `self`. `prev_ptr_write` points to |
Member
There was a problem hiding this comment.
We should make a note that next_write is incremented at most once per loop somewhere in here.
| // `prev_ptr_write` is never less than 0 and is inside the slice. | ||
| // This fulfils the requirements for dereferencing `ptr_read`, `prev_ptr_write` | ||
| // and `ptr_write`, and for using `ptr.add(next_read)`, `ptr.add(next_write - 1)` | ||
| // and `prev_ptr_write.offset(1)`. |
Member
There was a problem hiding this comment.
FWIW some of these constructions seem pretty strange, I wouldn't want to edit the code in this PR but ptr.add(next_write - 1) seems like it should be next_write.sub(1).
| T: Copy, | ||
| { | ||
| assert_eq!(self.len(), src.len(), "destination and source slices have different lengths"); | ||
| // SAFETY: `self` is valid for `self.len()` bytes by definition, and `src` was |
Member
There was a problem hiding this comment.
Suggested change
| // SAFETY: `self` is valid for `self.len()` bytes by definition, and `src` was | |
| // SAFETY: `self` is valid for `self.len()` elements by definition, and `src` was |
| { | ||
| assert_eq!(self.len(), src.len(), "destination and source slices have different lengths"); | ||
| // SAFETY: `self` is valid for `self.len()` bytes by definition, and `src` was | ||
| // checked to have the same length. Both slices cannot be overlapping because |
Member
There was a problem hiding this comment.
Suggested change
| // checked to have the same length. Both slices cannot be overlapping because | |
| // checked to have the same length. The slices cannot overlap because |
| assert_eq!(self.len(), src.len(), "destination and source slices have different lengths"); | ||
| // SAFETY: `self` is valid for `self.len()` bytes by definition, and `src` was | ||
| // checked to have the same length. Both slices cannot be overlapping because | ||
| // Rust's mutable references are exclusive. |
Member
There was a problem hiding this comment.
Suggested change
| // Rust's mutable references are exclusive. | |
| // mutable references are exclusive. |
Mark-Simulacrum
added
S-waiting-on-author
Collaborator
|
☔ The latest upstream changes (presumably #74330) made this pull request unmergeable. Please resolve the merge conflicts. |
Muirrum
added
S-waiting-on-author
|
Closing this after a discussion with the author since they don't have time to commit to this and it is small enough that can be redone later or someone could pick it up if needed. Thanks |
Dylan-DPC-zz
added
S-waiting-on-review
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Aug 12, 2020
…e-slice, r=LukasKalbertodt Document unsafety in library/core/src/slice/mod.rs Restart where rust-lang#73555 left off, helping with rust-lang#66219.
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Helps with #66219.
r? @Mark-Simulacrum