-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Safe memcpy for slices ([T]::copy_from_slice) #1419
Merged
Merged
Changes from 2 commits
Commits
Show all changes
12 commits
Select commit
Hold shift + click to select a range
618677f
std::slice::{ copy, set };
ubsan 08a87e4
Change a few things
ubsan ff919f1
Switch to fill, Add some language
ubsan 094f556
More edits from the crowd
ubsan 509a559
Rename `fill` to `fill_with`
ubsan 824d43c
Final name change; back to `fill`
ubsan a7bf742
Correction from bluss
ubsan b8625ae
Fix most of nagisa's complaints
ubsan 3ef78eb
Tone down the stuff about lowering to memset
ubsan da75c9b
Take out `[T]::fill`
ubsan 5c2c4c4
Merge remote-tracking branch 'upstream/master' into slice_copy_set
ubsan ea0ad1c
Rename copy_from -> copy_from_slice
ubsan File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains 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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,68 @@ | ||
- Feature Name: std::slice::{ copy, set }; | ||
- Start Date: (fill me in with today's date, YYYY-MM-DD) | ||
- RFC PR: (leave this empty) | ||
- Rust Issue: (leave this empty) | ||
|
||
# Summary | ||
[summary]: #summary | ||
|
||
Safe `memcpy` from one slice to another of the same type and length, and a safe | ||
`memset` of a slice of type `T: Copy`. | ||
|
||
# Motivation | ||
[motivation]: #motivation | ||
|
||
Currently, the only way to quickly copy from one non-`u8` slice to another is to | ||
use a loop, or unsafe methods like `std::ptr::copy_nonoverlapping`. This allows | ||
us to guarantee a `memcpy` for `Copy` types, and is safe. The only way to | ||
`memset` a slice, currently, is a loop, and we should expose a method to allow | ||
people to do this. This also completely gets rid of the point of | ||
`std::slice::bytes`, which means we can remove this deprecated and useless | ||
module. | ||
|
||
# Detailed design | ||
[design]: #detailed-design | ||
|
||
Add two functions to `std::slice`. | ||
|
||
```rust | ||
pub fn set<T: Copy>(slice: &mut [T], value: T); | ||
pub fn copy<T: Copy>(src: &[T], dst: &mut [T]); | ||
``` | ||
|
||
`set` loops through slice, setting each member to value. This will lower to a | ||
memset in all possible cases. | ||
|
||
`copy` panics if `src.len() != dst.len()`, then `memcpy`s the members from | ||
`src` to `dst`. | ||
|
||
# Drawbacks | ||
[drawbacks]: #drawbacks | ||
|
||
Two new functions in `std::slice`. `std::slice::set` *will not* be lowered to a | ||
`memset` in any case where the bytes of `value` are not all the same, as in | ||
|
||
```rust | ||
// let points: [f32; 16]; | ||
std::slice::set(&mut points, 1.0); // This is not lowered to a memset | ||
// (However, it is lowered to a simd loop, | ||
// which is what a memset is, in reality) | ||
``` | ||
|
||
# Alternatives | ||
[alternatives]: #alternatives | ||
|
||
We could name these functions something different. | ||
|
||
`memcpy` is also pretty weird, here. Panicking if the lengths differ is | ||
different from what came before; I believe it to be the safest path, because I | ||
think I'd want to know, personally, if I'm passing the wrong lengths to copy. | ||
However, `std::slice::bytes::copy_memory`, the function I'm basing this on, only | ||
panics if `dst.len() < src.len()`. So... room for discussion, here. | ||
|
||
These are necessary functions, in the opinion of the author. | ||
|
||
# Unresolved questions | ||
[unresolved]: #unresolved-questions | ||
|
||
None, as far as I can tell. |
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.
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.
They could be methods on slices, not necessarily free functions.