From 5c11392b14c3ed55b8166d3b790eeeb470764630 Mon Sep 17 00:00:00 2001 From: Scott McMurray Date: Sat, 1 Dec 2018 16:33:05 -0800 Subject: [PATCH 1/4] Redo the docs for Vec::set_len Inspired by the recent conversation on IRLO. --- src/liballoc/vec.rs | 80 ++++++++++++++++++++++++++++++--------------- 1 file changed, 53 insertions(+), 27 deletions(-) diff --git a/src/liballoc/vec.rs b/src/liballoc/vec.rs index f7a0bbdceafc9..4ff38247dae24 100644 --- a/src/liballoc/vec.rs +++ b/src/liballoc/vec.rs @@ -748,28 +748,64 @@ impl Vec { self } - /// Sets the length of a vector. + /// Forces the length of a vector to a particular value. /// - /// This will explicitly set the size of the vector, without actually - /// modifying its buffers, so it is up to the caller to ensure that the - /// vector is actually the specified size. + /// This is a low-level operation that maintains none of the normal + /// invariants of the type. Normally changing the length of a `Vec` + /// is done using one of the safe operations instead, such as + /// [`truncate`], [`resize`], [`extend`], or [`clear`]. /// - /// # Examples + /// [`truncate`]: #method.truncate + /// [`resize`]: #method.resize + /// [`extend`]: #method.extend-1 + /// [`clear`]: #method.clear /// - /// ``` - /// use std::ptr; + /// # Safety /// - /// let mut vec = vec!['r', 'u', 's', 't']; + /// - `new_len` must be less than or equal to `capacity()`. + /// - All elements between past the previous end up to the `new_len` + /// must be initialized. /// - /// unsafe { - /// ptr::drop_in_place(&mut vec[3]); - /// vec.set_len(3); + /// # Examples + /// + /// This method can be useful for situations in which the `Vec` is + /// serving as a buffer for other code, particularly over FFI: + /// + /// ```no_run + /// # #![allow(dead_code)] + /// # // This is just a minimal skeleton for the doc example; + /// # // don't use this as a starting point for a real library. + /// # pub struct StreamWrapper { strm: *mut std::ffi::c_void } + /// # const Z_OK: i32 = 0; + /// # extern "C" { + /// # fn deflateGetDictionary( + /// # strm: *mut std::ffi::c_void, + /// # dictionary: *mut u8, + /// # dictLength: *mut usize, + /// # ) -> i32; + /// # } + /// # impl StreamWrapper { + /// pub fn get_dictionary(&self) -> Option> { + /// // Per the docs, "32768 bytes is always enough". + /// let mut dict = Vec::with_capacity(32_768); + /// let mut dict_length = 0; + /// unsafe { + /// // Make the FFI call... + /// let r = deflateGetDictionary(self.strm, dict.as_mut_ptr(), &mut dict_length); + /// if r == Z_OK { + /// // ...and update the length to what was initialized. + /// dict.set_len(dict_length); + /// Some(dict) + /// } else { + /// None + /// } + /// } /// } - /// assert_eq!(vec, ['r', 'u', 's']); + /// # } /// ``` /// - /// In this example, there is a memory leak since the memory locations - /// owned by the inner vectors were not freed prior to the `set_len` call: + /// While the following example is sound, there is a memory leak since + /// the inner vectors were not freed prior to the `set_len` call: /// /// ``` /// let mut vec = vec![vec![1, 0, 0], @@ -780,21 +816,11 @@ impl Vec { /// } /// ``` /// - /// In this example, the vector gets expanded from zero to four items - /// without any memory allocations occurring, resulting in vector - /// values of unallocated memory: - /// - /// ``` - /// let mut vec: Vec = Vec::new(); - /// - /// unsafe { - /// vec.set_len(4); - /// } - /// ``` + /// (Instead, one would normally use [`clear`] in this situation.) #[inline] #[stable(feature = "rust1", since = "1.0.0")] - pub unsafe fn set_len(&mut self, len: usize) { - self.len = len; + pub unsafe fn set_len(&mut self, new_len: usize) { + self.len = new_len; } /// Removes an element from the vector and returns it. From ac642aba074969860f9fb53fb9e2d52f0e0203cb Mon Sep 17 00:00:00 2001 From: Scott McMurray Date: Tue, 11 Dec 2018 22:17:35 -0800 Subject: [PATCH 2/4] Update the comment some more following CR feedback --- src/liballoc/vec.rs | 20 +++++++++++--------- 1 file changed, 11 insertions(+), 9 deletions(-) diff --git a/src/liballoc/vec.rs b/src/liballoc/vec.rs index 4ff38247dae24..b987b94500fc4 100644 --- a/src/liballoc/vec.rs +++ b/src/liballoc/vec.rs @@ -748,10 +748,10 @@ impl Vec { self } - /// Forces the length of a vector to a particular value. + /// Forces the length of the vector to `new_len`. /// /// This is a low-level operation that maintains none of the normal - /// invariants of the type. Normally changing the length of a `Vec` + /// invariants of the type. Normally changing the length of a vector /// is done using one of the safe operations instead, such as /// [`truncate`], [`resize`], [`extend`], or [`clear`]. /// @@ -762,14 +762,15 @@ impl Vec { /// /// # Safety /// - /// - `new_len` must be less than or equal to `capacity()`. - /// - All elements between past the previous end up to the `new_len` - /// must be initialized. + /// - `new_len` must be less than or equal to [`capacity()`]. + /// - The elements at `old_len..new_len` must be initialized. + /// + /// [`capacity()`]: #method.capacity /// /// # Examples /// - /// This method can be useful for situations in which the `Vec` is - /// serving as a buffer for other code, particularly over FFI: + /// This method can be useful for situations in which the vector + /// is serving as a buffer for other code, particularly over FFI: /// /// ```no_run /// # #![allow(dead_code)] @@ -786,7 +787,7 @@ impl Vec { /// # } /// # impl StreamWrapper { /// pub fn get_dictionary(&self) -> Option> { - /// // Per the docs, "32768 bytes is always enough". + /// // Per the FFI method's docs, "32768 bytes is always enough". /// let mut dict = Vec::with_capacity(32_768); /// let mut dict_length = 0; /// unsafe { @@ -816,7 +817,8 @@ impl Vec { /// } /// ``` /// - /// (Instead, one would normally use [`clear`] in this situation.) + /// Normally, here, one would use [`clear`] instead to correctly drop + /// the contents and thus not leak memory. #[inline] #[stable(feature = "rust1", since = "1.0.0")] pub unsafe fn set_len(&mut self, new_len: usize) { From 61fb909559bc61e2179f3ea7b62b60e2e7df3ac0 Mon Sep 17 00:00:00 2001 From: Mazdak Farrokhzad Date: Wed, 2 Jan 2019 21:05:37 -0800 Subject: [PATCH 3/4] Update src/liballoc/vec.rs Add @centril's comment Co-Authored-By: scottmcm --- src/liballoc/vec.rs | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/liballoc/vec.rs b/src/liballoc/vec.rs index b987b94500fc4..30bcc034221d3 100644 --- a/src/liballoc/vec.rs +++ b/src/liballoc/vec.rs @@ -790,6 +790,10 @@ impl Vec { /// // Per the FFI method's docs, "32768 bytes is always enough". /// let mut dict = Vec::with_capacity(32_768); /// let mut dict_length = 0; + /// // SAFETY: When `deflateGetDictionary` returns `Z_OK`, it holds that: + /// // 1. `dict_length` elements were initialized. + /// // 2. `dict_length` <= the capacity (32_768) + /// // which makes `set_len` safe to call. /// unsafe { /// // Make the FFI call... /// let r = deflateGetDictionary(self.strm, dict.as_mut_ptr(), &mut dict_length); From 5052197e447a2279a63e8ef06179ca01b657eb9b Mon Sep 17 00:00:00 2001 From: Mazdak Farrokhzad Date: Wed, 9 Jan 2019 04:17:24 +0100 Subject: [PATCH 4/4] explain safety for vec.set_len(0) --- src/liballoc/vec.rs | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/liballoc/vec.rs b/src/liballoc/vec.rs index 30bcc034221d3..0da00b70f9e51 100644 --- a/src/liballoc/vec.rs +++ b/src/liballoc/vec.rs @@ -816,6 +816,9 @@ impl Vec { /// let mut vec = vec![vec![1, 0, 0], /// vec![0, 1, 0], /// vec![0, 0, 1]]; + /// // SAFETY: + /// // 1. `old_len..0` is empty so no elements need to be initialized. + /// // 2. `0 <= capacity` always holds whatever `capacity` is. /// unsafe { /// vec.set_len(0); /// }