[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <ZrNI4_z4v4pKroaR@pollux>
Date: Wed, 7 Aug 2024 12:13:55 +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,
acurrid@...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 v4 08/28] rust: types: implement `Unique<T>`
On Wed, Aug 07, 2024 at 07:27:43AM +0000, Benno Lossin wrote:
> On 07.08.24 01:12, Danilo Krummrich wrote:
> > On Tue, Aug 06, 2024 at 05:22:21PM +0000, Benno Lossin wrote:
> >> On 05.08.24 17:19, Danilo Krummrich wrote:
> >>> +impl<T: Sized> Unique<T> {
> >>> + /// Creates a new `Unique` that is dangling, but well-aligned.
> >>> + ///
> >>> + /// This is useful for initializing types which lazily allocate, like
> >>> + /// `Vec::new` does.
> >>> + ///
> >>> + /// Note that the pointer value may potentially represent a valid pointer to
> >>> + /// a `T`, which means this must not be used as a "not yet initialized"
> >>> + /// sentinel value. Types that lazily allocate must track initialization by
> >>> + /// some other means.
> >>> + #[must_use]
> >>> + #[inline]
> >>> + pub const fn dangling() -> Self {
> >>> + Unique {
> >>> + pointer: NonNull::dangling(),
> >>> + _marker: PhantomData,
> >>> + }
> >>> + }
> >>
> >> I think I already asked this, but the code until this point is copied
> >> from the rust stdlib and nowhere cited, does that work with the
> >> licensing?
> >>
> >> I also think that the code above could use some improvements:
> >> - add an `# Invariants` section with appropriate invariants (what are
> >> they supposed to be?)
> >> - Do we really want this type to be public and exported from the kernel
> >> crate? I think it would be better if it were crate-private.
> >> - What do we gain from having this type? As I learned recently, the
> >> `Unique` type from `core` doesn't actually put the `noalias` onto
> >> `Box` and `Vec`. The functions are mostly delegations to `NonNull`, so
> >> if the only advantages are that `Send` and `Sync` are already
> >> implemented, then I think we should drop this.
> >
> > I originally introduced it for the reasons described in [1], but mainly to make
> > clear that the owner of this thing also owns the memory behind the pointer and
> > the `Send` and `Sync` stuff you already mentioned.
>
> I would prefer if we make that explicit, since it is rather error-prone
> when creating new pointer types (and one should have to think about
> thread safety).
Again, fine for me. If no one else has objections I'll just drop `Unique`.
>
> ---
> Cheers,
> Benno
>
> > If no one else has objections we can also just drop it. Personally, I'm fine
> > either way.
> >
> > [1] https://docs.rs/rust-libcore/latest/core/ptr/struct.Unique.html
>
Powered by blists - more mailing lists