Skip to content

Commit 2e60288

Browse files
authored
Rollup merge of #133598 - ChayimFriedman2:get-many-mut-detailed-err, r=scottmcm
Change `GetManyMutError` to match T-libs-api decision That is, differentiate between out-of-bounds and overlapping indices, and remove the generic parameter `N`. I also exported `GetManyMutError` from `alloc` (and `std`), which was apparently forgotten. Changing the error to carry additional details means LLVM no longer generates separate short-circuiting branches for the checks, instead it generates one branch at the end. I therefore changed the code to use early returns to make LLVM generate jumps. Benchmark results between the approaches are somewhat mixed, but I chose this approach because it is significantly faster with ranges and also faster with `unwrap()`. Benchmark (`jumps` refer to short-circuiting, `acc` is not short-circuiting): ```rust use criterion::{black_box, criterion_group, criterion_main, Criterion}; use my_crate::{get_many_check_valid_acc, get_many_check_valid_jumps, GetManyMutError}; mod externs { #[unsafe(no_mangle)] fn foo() {} #[unsafe(no_mangle)] fn bar() {} #[unsafe(no_mangle)] fn baz() {} } unsafe extern "C" { safe fn foo(); safe fn bar(); safe fn baz(); } fn bench_method(c: &mut Criterion) { c.bench_function("jumps two usize", |b| { b.iter(|| get_many_check_valid_jumps(&[black_box(1), black_box(5)], black_box(10))) }); c.bench_function("jumps two usize unwrap", |b| { b.iter(|| get_many_check_valid_jumps(&[black_box(1), black_box(5)], black_box(10)).unwrap()) }); c.bench_function("jumps two usize ok", |b| { b.iter(|| get_many_check_valid_jumps(&[black_box(1), black_box(5)], black_box(10)).ok()) }); c.bench_function("jumps three usize", |b| { b.iter(|| { get_many_check_valid_jumps(&[black_box(1), black_box(5), black_box(7)], black_box(10)) }) }); c.bench_function("jumps three usize match", |b| { b.iter(|| { match get_many_check_valid_jumps( &[black_box(1), black_box(5), black_box(7)], black_box(10), ) { Err(GetManyMutError::IndexOutOfBounds) => foo(), Err(GetManyMutError::OverlappingIndices) => bar(), Ok(()) => baz(), } }) }); c.bench_function("jumps two Range", |b| { b.iter(|| { get_many_check_valid_jumps( &[black_box(1)..black_box(5), black_box(7)..black_box(8)], black_box(10), ) }) }); c.bench_function("jumps two RangeInclusive", |b| { b.iter(|| { get_many_check_valid_jumps( &[black_box(1)..=black_box(5), black_box(7)..=black_box(8)], black_box(10), ) }) }); c.bench_function("acc two usize", |b| { b.iter(|| get_many_check_valid_acc(&[black_box(1), black_box(5)], black_box(10))) }); c.bench_function("acc two usize unwrap", |b| { b.iter(|| get_many_check_valid_acc(&[black_box(1), black_box(5)], black_box(10)).unwrap()) }); c.bench_function("acc two usize ok", |b| { b.iter(|| get_many_check_valid_acc(&[black_box(1), black_box(5)], black_box(10)).ok()) }); c.bench_function("acc three usize", |b| { b.iter(|| { get_many_check_valid_acc(&[black_box(1), black_box(5), black_box(7)], black_box(10)) }) }); c.bench_function("acc three usize match", |b| { b.iter(|| { match get_many_check_valid_jumps( &[black_box(1), black_box(5), black_box(7)], black_box(10), ) { Err(GetManyMutError::IndexOutOfBounds) => foo(), Err(GetManyMutError::OverlappingIndices) => bar(), Ok(()) => baz(), } }) }); c.bench_function("acc two Range", |b| { b.iter(|| { get_many_check_valid_acc( &[black_box(1)..black_box(5), black_box(7)..black_box(8)], black_box(10), ) }) }); c.bench_function("acc two RangeInclusive", |b| { b.iter(|| { get_many_check_valid_acc( &[black_box(1)..=black_box(5), black_box(7)..=black_box(8)], black_box(10), ) }) }); } criterion_group!(benches, bench_method); criterion_main!(benches); ``` Benchmark results: ```none jumps two usize time: [586.44 ps 590.20 ps 594.50 ps] jumps two usize unwrap time: [390.44 ps 393.63 ps 397.44 ps] jumps two usize ok time: [585.52 ps 591.74 ps 599.38 ps] jumps three usize time: [976.51 ps 983.79 ps 991.51 ps] jumps three usize match time: [390.82 ps 393.80 ps 397.07 ps] jumps two Range time: [1.2583 ns 1.2640 ns 1.2695 ns] jumps two RangeInclusive time: [1.2673 ns 1.2770 ns 1.2877 ns] acc two usize time: [592.63 ps 596.44 ps 600.52 ps] acc two usize unwrap time: [582.65 ps 587.07 ps 591.90 ps] acc two usize ok time: [581.59 ps 587.82 ps 595.71 ps] acc three usize time: [894.69 ps 901.23 ps 908.24 ps] acc three usize match time: [392.68 ps 395.73 ps 399.17 ps] acc two Range time: [1.5531 ns 1.5617 ns 1.5711 ns] acc two RangeInclusive time: [1.5746 ns 1.5840 ns 1.5939 ns] ```
2 parents fe516ef + fa87a3e commit 2e60288

File tree

3 files changed

+34
-28
lines changed

3 files changed

+34
-28
lines changed

library/alloc/src/slice.rs

+2
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,8 @@ pub use core::slice::ArrayChunksMut;
2727
pub use core::slice::ArrayWindows;
2828
#[stable(feature = "inherent_ascii_escape", since = "1.60.0")]
2929
pub use core::slice::EscapeAscii;
30+
#[unstable(feature = "get_many_mut", issue = "104642")]
31+
pub use core::slice::GetManyMutError;
3032
#[stable(feature = "slice_get_slice", since = "1.28.0")]
3133
pub use core::slice::SliceIndex;
3234
#[cfg(not(no_global_oom_handling))]

library/core/src/error.rs

+1-1
Original file line numberDiff line numberDiff line change
@@ -1076,4 +1076,4 @@ impl Error for crate::time::TryFromFloatSecsError {}
10761076
impl Error for crate::ffi::FromBytesUntilNulError {}
10771077

10781078
#[unstable(feature = "get_many_mut", issue = "104642")]
1079-
impl<const N: usize> Error for crate::slice::GetManyMutError<N> {}
1079+
impl Error for crate::slice::GetManyMutError {}

library/core/src/slice/mod.rs

+31-27
Original file line numberDiff line numberDiff line change
@@ -4629,13 +4629,11 @@ impl<T> [T] {
46294629
pub fn get_many_mut<I, const N: usize>(
46304630
&mut self,
46314631
indices: [I; N],
4632-
) -> Result<[&mut I::Output; N], GetManyMutError<N>>
4632+
) -> Result<[&mut I::Output; N], GetManyMutError>
46334633
where
46344634
I: GetManyMutIndex + SliceIndex<Self>,
46354635
{
4636-
if !get_many_check_valid(&indices, self.len()) {
4637-
return Err(GetManyMutError { _private: () });
4638-
}
4636+
get_many_check_valid(&indices, self.len())?;
46394637
// SAFETY: The `get_many_check_valid()` call checked that all indices
46404638
// are disjunct and in bounds.
46414639
unsafe { Ok(self.get_many_unchecked_mut(indices)) }
@@ -4978,53 +4976,59 @@ impl<T, const N: usize> SlicePattern for [T; N] {
49784976
/// This will do `binomial(N + 1, 2) = N * (N + 1) / 2 = 0, 1, 3, 6, 10, ..`
49794977
/// comparison operations.
49804978
#[inline]
4981-
fn get_many_check_valid<I: GetManyMutIndex, const N: usize>(indices: &[I; N], len: usize) -> bool {
4979+
fn get_many_check_valid<I: GetManyMutIndex, const N: usize>(
4980+
indices: &[I; N],
4981+
len: usize,
4982+
) -> Result<(), GetManyMutError> {
49824983
// NB: The optimizer should inline the loops into a sequence
49834984
// of instructions without additional branching.
4984-
let mut valid = true;
49854985
for (i, idx) in indices.iter().enumerate() {
4986-
valid &= idx.is_in_bounds(len);
4986+
if !idx.is_in_bounds(len) {
4987+
return Err(GetManyMutError::IndexOutOfBounds);
4988+
}
49874989
for idx2 in &indices[..i] {
4988-
valid &= !idx.is_overlapping(idx2);
4990+
if idx.is_overlapping(idx2) {
4991+
return Err(GetManyMutError::OverlappingIndices);
4992+
}
49894993
}
49904994
}
4991-
valid
4995+
Ok(())
49924996
}
49934997

4994-
/// The error type returned by [`get_many_mut<N>`][`slice::get_many_mut`].
4998+
/// The error type returned by [`get_many_mut`][`slice::get_many_mut`].
49954999
///
49965000
/// It indicates one of two possible errors:
49975001
/// - An index is out-of-bounds.
4998-
/// - The same index appeared multiple times in the array.
5002+
/// - The same index appeared multiple times in the array
5003+
/// (or different but overlapping indices when ranges are provided).
49995004
///
50005005
/// # Examples
50015006
///
50025007
/// ```
50035008
/// #![feature(get_many_mut)]
5009+
/// use std::slice::GetManyMutError;
50045010
///
50055011
/// let v = &mut [1, 2, 3];
5006-
/// assert!(v.get_many_mut([0, 999]).is_err());
5007-
/// assert!(v.get_many_mut([1, 1]).is_err());
5012+
/// assert_eq!(v.get_many_mut([0, 999]), Err(GetManyMutError::IndexOutOfBounds));
5013+
/// assert_eq!(v.get_many_mut([1, 1]), Err(GetManyMutError::OverlappingIndices));
50085014
/// ```
50095015
#[unstable(feature = "get_many_mut", issue = "104642")]
5010-
// NB: The N here is there to be forward-compatible with adding more details
5011-
// to the error type at a later point
5012-
#[derive(Clone, PartialEq, Eq)]
5013-
pub struct GetManyMutError<const N: usize> {
5014-
_private: (),
5016+
#[derive(Debug, Clone, PartialEq, Eq)]
5017+
pub enum GetManyMutError {
5018+
/// An index provided was out-of-bounds for the slice.
5019+
IndexOutOfBounds,
5020+
/// Two indices provided were overlapping.
5021+
OverlappingIndices,
50155022
}
50165023

50175024
#[unstable(feature = "get_many_mut", issue = "104642")]
5018-
impl<const N: usize> fmt::Debug for GetManyMutError<N> {
5025+
impl fmt::Display for GetManyMutError {
50195026
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
5020-
f.debug_struct("GetManyMutError").finish_non_exhaustive()
5021-
}
5022-
}
5023-
5024-
#[unstable(feature = "get_many_mut", issue = "104642")]
5025-
impl<const N: usize> fmt::Display for GetManyMutError<N> {
5026-
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
5027-
fmt::Display::fmt("an index is out of bounds or appeared multiple times in the array", f)
5027+
let msg = match self {
5028+
GetManyMutError::IndexOutOfBounds => "an index is out of bounds",
5029+
GetManyMutError::OverlappingIndices => "there were overlapping indices",
5030+
};
5031+
fmt::Display::fmt(msg, f)
50285032
}
50295033
}
50305034

0 commit comments

Comments
 (0)