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.

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
Changes from 2 commits
Commits
File filter...
Filter file types
Jump to…
Jump to file or symbol
Failed to load files and symbols.

Always

Just for now

@@ -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,

This comment has been minimized.

Copy link
@cramertj

cramertj Jun 1, 2019

Member

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.

This comment has been minimized.

Copy link
@SimonSapin

SimonSapin Jun 2, 2019

Contributor

Wouldn’t the method still be generic over T?

This comment has been minimized.

Copy link
@nagisa

nagisa Jun 2, 2019

Contributor

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.

This comment has been minimized.

Copy link
@cramertj

cramertj Jun 3, 2019

Member

@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...

This comment has been minimized.

Copy link
@nagisa

nagisa Jun 3, 2019

Contributor

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

This comment has been minimized.

Copy link
@cramertj

cramertj Jun 3, 2019

Member

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.

@@ -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,
);
}
@@ -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)]
@@ -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]
@@ -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!";
// 2 is greater than 1, so this range is invalid.
This conversation was marked as resolved by kennytm

This comment has been minimized.

Copy link
@oconnor663

oconnor663 Jun 3, 2019

Contributor
Suggested change
// 2 is greater than 1, so this range is invalid.
// 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() {
ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.