linux-kernel - Re: [PATCH v6 14/26] rust: alloc: implement `IntoIterator` for `Vec`

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <ZuF_x__jiP6KHlvZ@pollux>
Date: Wed, 11 Sep 2024 13:32:23 +0200
From: Danilo Krummrich <dakr@...nel.org>
To: Benno Lossin <benno.lossin@...ton.me>
Cc: ojeda@...nel.org, alex.gaynor@...il.com, wedsonaf@...il.com,
	boqun.feng@...il.com, gary@...yguo.net, bjorn3_gh@...tonmail.com,
	a.hindborg@...sung.com, aliceryhl@...gle.com,
	akpm@...ux-foundation.org, daniel.almeida@...labora.com,
	faith.ekstrand@...labora.com, boris.brezillon@...labora.com,
	lina@...hilina.net, mcanal@...lia.com, zhiw@...dia.com,
	cjia@...dia.com, jhubbard@...dia.com, airlied@...hat.com,
	ajanulgu@...hat.com, lyude@...hat.com, linux-kernel@...r.kernel.org,
	rust-for-linux@...r.kernel.org, linux-mm@...ck.org
Subject: Re: [PATCH v6 14/26] rust: alloc: implement `IntoIterator` for `Vec`

On Wed, Sep 11, 2024 at 08:52:03AM +0000, Benno Lossin wrote:
> On 11.09.24 01:39, Danilo Krummrich wrote:
> > On Tue, Sep 10, 2024 at 08:04:27PM +0000, Benno Lossin wrote:
> >> On 16.08.24 02:10, Danilo Krummrich wrote:
> >>> +/// [`IntoIterator`] trait).
> >>> +///
> >>> +/// # Examples
> >>> +///
> >>> +/// ```
> >>> +/// let v = kernel::kvec![0, 1, 2]?;
> >>> +/// let iter = v.into_iter();
> >>> +///
> >>> +/// # Ok::<(), Error>(())
> >>> +/// ```
> >>> +pub struct IntoIter<T, A: Allocator> {
> >>> +    ptr: *mut T,
> >>> +    buf: NonNull<T>,
> >>
> >> No invariants for these two fields?
> > 
> > Suggestions?
> 
> When determining the invariants, I look at the places where you would
> want to use them, ie the `SAFETY` comments that use these fields:
> - for `buf` you only use it to free the backing allocation, so you only
>   need that it has been allocated by `A` if `cap != 0`.
> - for `ptr` you need that it is valid for reads for `size_of::<T>() *
>   length` bytes.
> 
> So I would put those two things into invariants.
> 
> >>> +    len: usize,
> >>> +    cap: usize,
> >>> +    _p: PhantomData<A>,
> >>> +}
> >>> +
> >>> +impl<T, A> IntoIter<T, A>
> >>> +where
> >>> +    A: Allocator,
> >>> +{
> >>> +    fn as_raw_mut_slice(&mut self) -> *mut [T] {
> >>> +        ptr::slice_from_raw_parts_mut(self.ptr, self.len)
> >>> +    }
> >>> +}
> >>> +
> >>> +impl<T, A> Iterator for IntoIter<T, A>
> >>> +where
> >>> +    A: Allocator,
> >>> +{
> >>> +    type Item = T;
> >>> +
> >>> +    /// # Examples
> >>> +    ///
> >>> +    /// ```
> >>> +    /// let v = kernel::kvec![1, 2, 3]?;
> >>> +    /// let mut it = v.into_iter();
> >>> +    ///
> >>> +    /// assert_eq!(it.next(), Some(1));
> >>> +    /// assert_eq!(it.next(), Some(2));
> >>> +    /// assert_eq!(it.next(), Some(3));
> >>> +    /// assert_eq!(it.next(), None);
> >>> +    ///
> >>> +    /// # Ok::<(), Error>(())
> >>> +    /// ```
> >>
> >> AFAIK documentation on functions in trait implementations won't show up
> >> in rustdoc (I just checked this). So I would remove it.
> > 
> > They don't, but the KUnit tests are still executed. :)
> 
> Oh I see, then may I suggest moving them to the module documentation or
> put them onto `Vec`, that way people can also read them :)

Hm, I'd rather keep them close on the functions they're testing. Those examples
probably don't have a huge documentation purpose. As you've said, the trait is
documented already and has examples.

> 
> ---
> Cheers,
> Benno
> 
>