Skip to content
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.

Sign up for GitHub

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

Stabilize copy_within #61398

Merged
merged 3 commits into from
Jun 13, 2019
Merged

Stabilize copy_within #61398

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
be6fc6a
Stabilize copy_within
kennytm May 31, 2019
aac9bc5
copy_within: replace element access by pointer arithmetic to avoid UB
kennytm Jun 2, 2019
427f1a4
Update src/libcore/tests/slice.rs
kennytm Jun 3, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 3 additions & 4 deletions src/libcore/slice/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2146,14 +2146,13 @@ impl<T> [T] {
/// Copying four bytes within a slice:
///
/// ```
/// # #![feature(copy_within)]
/// let mut bytes = *b"Hello, World!";
///
/// bytes.copy_within(1..5, 8);
///
/// assert_eq!(&bytes, b"Hello, Wello!");
/// ```
#[unstable(feature = "copy_within", issue = "54236")]
#[stable(feature = "copy_within", since = "1.37.0")]
pub fn copy_within<R: ops::RangeBounds<usize>>(&mut self, src: R, dest: usize)
where
T: Copy,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

drive-by unrelated to stabilization: seems like we shouldn't be monomorphizing this whole function on R, given that we only need .start_bound() and .end_bound(), which we can call first and pass into a non-generic version.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn’t the method still be generic over T?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LLVM will deduplicate identical functions anyway. Hard to see this coming at a compilation time cost and if it ever does in practice, it would be great to fix it in general rather than implementing hacks for specific functions in libcore.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nagisa the point is that they wouldn't be identical functions because they'd have different types for R. So we'd generate separate copies for Range, RangeFrom, RangeTo, RangeInclusive, etc...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cramertj if the generated IR after inlining is identical, it will get merged. LLVM does not care about the type system.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My point is that I don't think the generated IR would be identical. RangeFrom<T> and Range<T> don't have the same representation, so converting them to start and end bounds would need to be split out into a separate function in order for the inner body of the functions to be unified.

Expand All @@ -2178,8 +2177,8 @@ impl<T> [T] {
assert!(dest <= self.len() - count, "dest is out of bounds");
unsafe {
ptr::copy(
self.get_unchecked(src_start),
self.get_unchecked_mut(dest),
self.as_ptr().add(src_start),
self.as_mut_ptr().add(dest),
count,
);
}
Expand Down
1 change: 0 additions & 1 deletion src/libcore/tests/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,6 @@
#![feature(inner_deref)]
#![feature(slice_internals)]
#![feature(slice_partition_dedup)]
#![feature(copy_within)]
#![feature(int_error_matching)]
#![feature(const_fn)]
#![warn(rust_2018_idioms)]
Expand Down
14 changes: 14 additions & 0 deletions src/libcore/tests/slice.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1512,6 +1512,13 @@ fn test_copy_within() {
let mut bytes = *b"Hello, World!";
bytes.copy_within(.., 0);
assert_eq!(&bytes, b"Hello, World!");

// Ensure that copying at the end of slice won't cause UB.
let mut bytes = *b"Hello, World!";
bytes.copy_within(13..13, 5);
assert_eq!(&bytes, b"Hello, World!");
bytes.copy_within(5..5, 13);
assert_eq!(&bytes, b"Hello, World!");
}

#[test]
Expand All @@ -1536,6 +1543,13 @@ fn test_copy_within_panics_src_inverted() {
// 2 is greater than 1, so this range is invalid.
bytes.copy_within(2..1, 0);
}
#[test]
#[should_panic(expected = "attempted to index slice up to maximum usize")]
fn test_copy_within_panics_src_out_of_bounds() {
let mut bytes = *b"Hello, World!";
// an inclusive range ending at usize::max_value() would make src_end overflow
bytes.copy_within(usize::max_value()..=usize::max_value(), 0);
}

#[test]
fn test_is_sorted() {
Expand Down