From 2706ae1a926c21fb0ee5bb7baaf1c6ad2f8aa1da Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sebastian=20Dr=C3=B6ge?= Date: Mon, 1 Feb 2021 10:07:48 +0200 Subject: [PATCH 1/3] Add SAFETY comment to the `TrustedLen` impl of `vec::Drain` --- library/alloc/src/vec/drain.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/library/alloc/src/vec/drain.rs b/library/alloc/src/vec/drain.rs index fb32d144f872c..47b20dff16bb2 100644 --- a/library/alloc/src/vec/drain.rs +++ b/library/alloc/src/vec/drain.rs @@ -149,6 +149,8 @@ impl ExactSizeIterator for Drain<'_, T, A> { } #[unstable(feature = "trusted_len", issue = "37572")] +// SAFETY: `Drain` simply forwards to the underlying slice iterator, which implements `TrustedLen` +// so the required properties are all preserved. unsafe impl TrustedLen for Drain<'_, T, A> {} #[stable(feature = "fused", since = "1.26.0")] From b3fcb750c5f78b42033fac761377144647633038 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sebastian=20Dr=C3=B6ge?= Date: Mon, 1 Feb 2021 10:11:45 +0200 Subject: [PATCH 2/3] Implement `TrustedRandomAccess` for `vec::Drain` --- library/alloc/src/vec/drain.rs | 32 +++++++++++++++++++++++++++++++- 1 file changed, 31 insertions(+), 1 deletion(-) diff --git a/library/alloc/src/vec/drain.rs b/library/alloc/src/vec/drain.rs index 47b20dff16bb2..8772c3a643fce 100644 --- a/library/alloc/src/vec/drain.rs +++ b/library/alloc/src/vec/drain.rs @@ -1,6 +1,6 @@ use crate::alloc::{Allocator, Global}; use core::fmt; -use core::iter::{FusedIterator, TrustedLen}; +use core::iter::{FusedIterator, TrustedLen, TrustedRandomAccess}; use core::mem::{self}; use core::ptr::{self, NonNull}; use core::slice::{self}; @@ -89,6 +89,19 @@ impl Iterator for Drain<'_, T, A> { fn size_hint(&self) -> (usize, Option) { self.iter.size_hint() } + + #[doc(hidden)] + unsafe fn __iterator_get_unchecked(&mut self, idx: usize) -> T + where + Self: TrustedRandomAccess, + { + // SAFETY: `TrustedRandomAccess` requires that `idx` is in bounds and that + // each `idx` is only accessed once. Forwarding to the slice iterator's + // implementation is thus safe, and reading the value is safe because + // `Self: TrustedRandomAccess` implies `T: Copy` so the `Drop` impl below + // won't cause each item to be dropped twice. + unsafe { ptr::read(self.iter.__iterator_get_unchecked(idx) as *const _) } + } } #[stable(feature = "drain", since = "1.6.0")] @@ -153,5 +166,22 @@ impl ExactSizeIterator for Drain<'_, T, A> { // so the required properties are all preserved. unsafe impl TrustedLen for Drain<'_, T, A> {} +#[doc(hidden)] +#[unstable(feature = "trusted_random_access", issue = "none")] +// SAFETY: `Drain` forwards to the underlying slice iterator, which implements `TrustedRandomAccess`, +// and then reads the items instead of just returning a reference. As `TrustedRandomAccess` +// requires each index to be accessed only once, this is safe to do here. +// +// T: Copy as approximation for !Drop since get_unchecked does not advance self.iter +// and as a result the `Drop` impl above would otherwise cause items to be dropped twice. +unsafe impl TrustedRandomAccess for Drain<'_, T, A> +where + T: Copy, +{ + fn may_have_side_effect() -> bool { + false + } +} + #[stable(feature = "fused", since = "1.26.0")] impl FusedIterator for Drain<'_, T, A> {} From 1d33db3dc0c8ee4db0ab1997a6d5d836f2ef1b75 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sebastian=20Dr=C3=B6ge?= Date: Mon, 1 Feb 2021 10:14:20 +0200 Subject: [PATCH 3/3] Only exhaust the slice iterator in `vec::Drain` if the items need dropping Otherwise this simply causes advancing of the iterator with no other effects, which is unnecessary work. --- library/alloc/src/vec/drain.rs | 20 ++++++++++++-------- 1 file changed, 12 insertions(+), 8 deletions(-) diff --git a/library/alloc/src/vec/drain.rs b/library/alloc/src/vec/drain.rs index 8772c3a643fce..ccce7da059088 100644 --- a/library/alloc/src/vec/drain.rs +++ b/library/alloc/src/vec/drain.rs @@ -121,9 +121,11 @@ impl Drop for Drain<'_, T, A> { impl<'r, 'a, T, A: Allocator> Drop for DropGuard<'r, 'a, T, A> { fn drop(&mut self) { - // Continue the same loop we have below. If the loop already finished, this does - // nothing. - self.0.for_each(drop); + if mem::needs_drop::() { + // Continue the same loop we have below. If the loop already finished, this does + // nothing. + self.0.for_each(drop); + } if self.0.tail_len > 0 { unsafe { @@ -142,11 +144,13 @@ impl Drop for Drain<'_, T, A> { } } - // exhaust self first - while let Some(item) = self.next() { - let guard = DropGuard(self); - drop(item); - mem::forget(guard); + // exhaust self first if dropping of the items is required + if mem::needs_drop::() { + while let Some(item) = self.next() { + let guard = DropGuard(self); + drop(item); + mem::forget(guard); + } } // Drop a `DropGuard` to move back the non-drained tail of `self`.