Re: [PATCH] rust: introduce KVec::pop()

[Danilo Krummrich <dakr@kernel.org>]

On Mon, Dec 02, 2024 at 09:57:58AM +0100, Bernhard Kauer wrote:
> Provides a simple stack together with push().

Your commit message should also explain the motivation of the patch, i.e. what
do you need this new function for.

> 
> Signed-off-by: Bernhard Kauer <bk@alpico.io>
> ---
>  rust/kernel/alloc/kvec.rs | 28 ++++++++++++++++++++++++++++
>  1 file changed, 28 insertions(+)
> 
> diff --git a/rust/kernel/alloc/kvec.rs b/rust/kernel/alloc/kvec.rs
> index ae9d072741ce..ba6c265ad5c5 100644
> --- a/rust/kernel/alloc/kvec.rs
> +++ b/rust/kernel/alloc/kvec.rs
> @@ -302,6 +302,34 @@ pub fn push(&mut self, v: T, flags: Flags) -> Result<(), AllocError> {
>          Ok(())
>      }
>  
> +    /// Removes the last element from the [`Vec`] instance.

It not only removes it, but also returns the corresponding element (or `None` if
the vector is empty). Please add this to the documentation.

It would also be worth noting that users may want to shrink the vector after
removing elements, however, we don't implement the `shrink_*` functions yet.

> +    ///
> +    /// # Examples
> +    ///
> +    /// ```
> +    /// let mut v = KVec::new();
> +    /// v.push(1, GFP_KERNEL)?;
> +    /// v.push(2, GFP_KERNEL)?;
> +    /// assert_eq!(Some(2), v.pop());
> +    /// assert_eq!(Some(1), v.pop());
> +    /// assert_eq!(None, v.pop());
> +    /// assert!(v.is_empty());
> +    /// ```
> +    pub fn pop(&mut self) -> Option<T> {
> +        if self.len() == 0 {
> +            return None;
> +        }
> +        self.len -= 1;
> +
> +        // SAFETY:
> +        // - `ptr` is valid because there is at least one entry stored.
> +        let ptr = unsafe { self.as_mut_ptr().add(self.len) };
> +
> +        // SAFETY:
> +        // - `ptr` is properly aligned and valid for reads.
> +        Some(unsafe { core::ptr::read(ptr) })
> +    }
> +
>      /// Creates a new [`Vec`] instance with at least the given capacity.
>      ///
>      /// # Examples
> -- 
> 2.45.2
>