From 6da4df9fc95c1da7373f49c07742a42aaf840198 Mon Sep 17 00:00:00 2001 From: gnzlbg Date: Fri, 13 Sep 2019 17:14:57 +0200 Subject: [PATCH] Make the semantics of Vec::truncate(N) consistent with slices. This commit simplifies the implementation of `Vec::truncate(N)` and makes its semantics identical to dropping the `[vec.len() - N..]` sub-slice tail of the vector, which is the same behavior as dropping a vector containing the same sub-slice. This changes two unspecified aspects of `Vec::truncate` behavior: * the drop order, from back-to-front to front-to-back, * the behavior of `Vec::truncate` on panics: if dropping one element of the tail panics, currently, `Vec::truncate` panics, but with this PR all other elements are still dropped, and if dropping a second element of the tail panics, with this PR, the program aborts. Programs can trivially observe both changes. For example ([playground](https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=7bef575b83b06e82b3e3529e4edbcac7)): ```rust fn main() { struct Bomb(usize); impl Drop for Bomb { fn drop(&mut self) { panic!(format!("{}", self.0)); } } let mut v = vec![Bomb(0), Bomb(1)]; std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| { v.truncate(0); })); assert_eq!(v.len(), 1); std::mem::forget(v); } ``` panics printing `1` today and succeeds. With this change, it panics printing `0` first (due to the drop order change), and then aborts with a double-panic printing `1`, just like dropping the `[Bomb(0), Bomb(1)]` slice does, or dropping `vec![Bomb(0), Bomb(1)]` does. --- src/liballoc/vec.rs | 34 ++++++++++++---------------------- 1 file changed, 12 insertions(+), 22 deletions(-) diff --git a/src/liballoc/vec.rs b/src/liballoc/vec.rs index c513658c842..64c49ccfefc 100644 --- a/src/liballoc/vec.rs +++ b/src/liballoc/vec.rs @@ -685,25 +685,20 @@ pub fn into_boxed_slice(mut self) -> Box<[T]> { /// [`drain`]: #method.drain #[stable(feature = "rust1", since = "1.0.0")] pub fn truncate(&mut self, len: usize) { - if mem::needs_drop::() { - let current_len = self.len; - unsafe { - let mut ptr = self.as_mut_ptr().add(self.len); - // Set the final length at the end, keeping in mind that - // dropping an element might panic. Works around a missed - // optimization, as seen in the following issue: - // https://github.com/rust-lang/rust/issues/51802 - let mut local_len = SetLenOnDrop::new(&mut self.len); - - // drop any extra elements - for _ in len..current_len { - local_len.decrement_len(1); - ptr = ptr.offset(-1); - ptr::drop_in_place(ptr); - } + // This is safe because: + // + // * the slice passed to `drop_in_place` is valid; the `len > self.len` + // case avoids creating an invalid slice, and + // * the `len` of the vector is shrunk before calling `drop_in_place`, + // such that no value will be dropped twice in case `drop_in_place` + // were to panic once (if it panics twice, the program aborts). + unsafe { + if len > self.len { + return; } - } else if len <= self.len { + let s = self.get_unchecked_mut(len..) as *mut _; self.len = len; + ptr::drop_in_place(s); } } @@ -1590,11 +1585,6 @@ fn new(len: &'a mut usize) -> Self { fn increment_len(&mut self, increment: usize) { self.local_len += increment; } - - #[inline] - fn decrement_len(&mut self, decrement: usize) { - self.local_len -= decrement; - } } impl Drop for SetLenOnDrop<'_> {