linux-kernel - Re: [PATCH v6 15/26] rust: alloc: implement `collect` for `IntoIter`

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: <503abe24-fbac-440a-a063-fc30a2a6bd77@proton.me>
Date: Wed, 11 Sep 2024 08:53:24 +0000
From: Benno Lossin <benno.lossin@...ton.me>
To: Danilo Krummrich <dakr@...nel.org>
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 15/26] rust: alloc: implement `collect` for `IntoIter`

On 11.09.24 02:22, Danilo Krummrich wrote:
> On Tue, Sep 10, 2024 at 08:12:24PM +0000, Benno Lossin wrote:
>> On 16.08.24 02:10, Danilo Krummrich wrote:
>>> +    /// Same as `Iterator::collect` but specialized for `Vec`'s `IntoIter`.
>>> +    ///
>>> +    /// Currently, we can't implement `FromIterator`. There are a couple of issues with this trait
>>> +    /// in the kernel, namely:
>>> +    ///
>>> +    /// - Rust's specialization feature is unstable. This prevents us to optimze for the special
>>> +    ///   case where `I::IntoIter` equals `Vec`'s `IntoIter` type.
>>> +    /// - We also can't use `I::IntoIter`'s type ID either to work around this, since `FromIterator`
>>> +    ///   doesn't require this type to be `'static`.
>>> +    /// - `FromIterator::from_iter` does return `Self` instead of `Result<Self, AllocError>`, hence
>>> +    ///   we can't properly handle allocation failures.
>>> +    /// - Neither `Iterator::collect` nor `FromIterator::from_iter` can handle additional allocation
>>> +    ///   flags.
>>> +    ///
>>> +    /// Instead, provide `IntoIter::collect`, such that we can at least convert a `IntoIter` into a
>>> +    /// `Vec` again.
>>
>> I think it's great that you include this in the code, but I don't think
>> that it should be visible in the documentation,
> 
> Why not? I think this information is valuable for users of this API.

If you want to keep it, then I don't mind, but I would still move it
underneath `Examples` and add a section header `# Implementation
Details` or similar.

---
Cheers,
Benno

>> can you move it under
>> the `Examples` section and turn it into normal comments?
>>
>>> +    ///
>>> +    /// Note that `IntoIter::collect` doesn't require `Flags`, since it re-uses the existing backing
>>> +    /// buffer. However, this backing buffer may be shrunk to the actual count of elements.
>>> +    ///
>>> +    /// # Examples
>>> +    ///
>>> +    /// ```
>>> +    /// let v = kernel::kvec![1, 2, 3]?;
>>> +    /// let mut it = v.into_iter();
>>> +    ///
>>> +    /// assert_eq!(it.next(), Some(1));
>>> +    ///
>>> +    /// let v = it.collect(GFP_KERNEL);
>>> +    /// assert_eq!(v, [2, 3]);
>>> +    ///
>>> +    /// # Ok::<(), Error>(())
>>> +    /// ```
>>> +    pub fn collect(self, flags: Flags) -> Vec<T, A> {