1. Patchew
  2. linux
  3. [PATCH 0/3] rust: xarray: add `insert` and `reserve`
  4. View patch

[PATCH 1/3] rust: xarray: use the prelude

Tamir Duberstein posted 3 patches 3 months, 1 week ago
There is a newer version of this series
[PATCH 1/3] rust: xarray: use the prelude
Posted by Tamir Duberstein 3 months, 1 week ago 
Using the prelude is customary in the kernel crate.

Signed-off-by: Tamir Duberstein <tamird@gmail.com>
---
 rust/kernel/xarray.rs | 34 ++++++++++++++++++++--------------
 1 file changed, 20 insertions(+), 14 deletions(-)

diff --git a/rust/kernel/xarray.rs b/rust/kernel/xarray.rs
index 75719e7bb491..436faad99c89 100644
--- a/rust/kernel/xarray.rs
+++ b/rust/kernel/xarray.rs
@@ -5,16 +5,15 @@
 //! C header: [`include/linux/xarray.h`](srctree/include/linux/xarray.h)
 
 use crate::{
-    alloc, bindings, build_assert,
-    error::{Error, Result},
+    alloc,
+    prelude::*,
     types::{ForeignOwnable, NotThreadSafe, Opaque},
 };
-use core::{iter, marker::PhantomData, mem, pin::Pin, ptr::NonNull};
-use pin_init::{pin_data, pin_init, pinned_drop, PinInit};
+use core::{iter, marker::PhantomData, mem, ptr::NonNull};
 
 /// An array which efficiently maps sparse integer indices to owned objects.
 ///
-/// This is similar to a [`crate::alloc::kvec::Vec<Option<T>>`], but more efficient when there are
+/// This is similar to a [`Vec<Option<T>>`], but more efficient when there are
 /// holes in the index space, and can be efficiently grown.
 ///
 /// # Invariants
@@ -104,16 +103,23 @@ pub fn new(kind: AllocKind) -> impl PinInit<Self> {
     fn iter(&self) -> impl Iterator<Item = NonNull<T::PointedTo>> + '_ {
         let mut index = 0;
 
-        // SAFETY: `self.xa` is always valid by the type invariant.
-        iter::once(unsafe {
-            bindings::xa_find(self.xa.get(), &mut index, usize::MAX, bindings::XA_PRESENT)
-        })
-        .chain(iter::from_fn(move || {
+        core::iter::Iterator::chain(
             // SAFETY: `self.xa` is always valid by the type invariant.
-            Some(unsafe {
-                bindings::xa_find_after(self.xa.get(), &mut index, usize::MAX, bindings::XA_PRESENT)
-            })
-        }))
+            iter::once(unsafe {
+                bindings::xa_find(self.xa.get(), &mut index, usize::MAX, bindings::XA_PRESENT)
+            }),
+            iter::from_fn(move || {
+                // SAFETY: `self.xa` is always valid by the type invariant.
+                Some(unsafe {
+                    bindings::xa_find_after(
+                        self.xa.get(),
+                        &mut index,
+                        usize::MAX,
+                        bindings::XA_PRESENT,
+                    )
+                })
+            }),
+        )
         .map_while(|ptr| NonNull::new(ptr.cast()))
     }
 

-- 
2.50.0
Re: [PATCH 1/3] rust: xarray: use the prelude
Posted by Boqun Feng 3 months, 1 week ago 
On Tue, Jul 01, 2025 at 12:27:17PM -0400, Tamir Duberstein wrote:
> Using the prelude is customary in the kernel crate.
> 
> Signed-off-by: Tamir Duberstein <tamird@gmail.com>
> ---
>  rust/kernel/xarray.rs | 34 ++++++++++++++++++++--------------
>  1 file changed, 20 insertions(+), 14 deletions(-)
> 
> diff --git a/rust/kernel/xarray.rs b/rust/kernel/xarray.rs
> index 75719e7bb491..436faad99c89 100644
> --- a/rust/kernel/xarray.rs
> +++ b/rust/kernel/xarray.rs
> @@ -5,16 +5,15 @@
>  //! C header: [`include/linux/xarray.h`](srctree/include/linux/xarray.h)
>  
>  use crate::{
> -    alloc, bindings, build_assert,
> -    error::{Error, Result},
> +    alloc,
> +    prelude::*,
>      types::{ForeignOwnable, NotThreadSafe, Opaque},
>  };
> -use core::{iter, marker::PhantomData, mem, pin::Pin, ptr::NonNull};
> -use pin_init::{pin_data, pin_init, pinned_drop, PinInit};
> +use core::{iter, marker::PhantomData, mem, ptr::NonNull};
>  
>  /// An array which efficiently maps sparse integer indices to owned objects.
>  ///
> -/// This is similar to a [`crate::alloc::kvec::Vec<Option<T>>`], but more efficient when there are
> +/// This is similar to a [`Vec<Option<T>>`], but more efficient when there are
>  /// holes in the index space, and can be efficiently grown.
>  ///
>  /// # Invariants
> @@ -104,16 +103,23 @@ pub fn new(kind: AllocKind) -> impl PinInit<Self> {
>      fn iter(&self) -> impl Iterator<Item = NonNull<T::PointedTo>> + '_ {
>          let mut index = 0;
>  
> -        // SAFETY: `self.xa` is always valid by the type invariant.
> -        iter::once(unsafe {
> -            bindings::xa_find(self.xa.get(), &mut index, usize::MAX, bindings::XA_PRESENT)
> -        })
> -        .chain(iter::from_fn(move || {
> +        core::iter::Iterator::chain(

Does this part come from using the prelude? If not, either we need to
split the patch or we need to mention it in the changelog at least.

Also since we `use core::iter` above, we can avoid the `core::` here.

Regards,
Boqun

>              // SAFETY: `self.xa` is always valid by the type invariant.
> -            Some(unsafe {
> -                bindings::xa_find_after(self.xa.get(), &mut index, usize::MAX, bindings::XA_PRESENT)
> -            })
> -        }))
> +            iter::once(unsafe {
> +                bindings::xa_find(self.xa.get(), &mut index, usize::MAX, bindings::XA_PRESENT)
> +            }),
> +            iter::from_fn(move || {
> +                // SAFETY: `self.xa` is always valid by the type invariant.
> +                Some(unsafe {
> +                    bindings::xa_find_after(
> +                        self.xa.get(),
> +                        &mut index,
> +                        usize::MAX,
> +                        bindings::XA_PRESENT,
> +                    )
> +                })
> +            }),
> +        )
>          .map_while(|ptr| NonNull::new(ptr.cast()))
>      }
>  
> 
> -- 
> 2.50.0
>
Re: [PATCH 1/3] rust: xarray: use the prelude
Posted by Tamir Duberstein 3 months, 1 week ago 
On Tue, Jul 1, 2025 at 12:35 PM Boqun Feng <boqun.feng@gmail.com> wrote:
>
> On Tue, Jul 01, 2025 at 12:27:17PM -0400, Tamir Duberstein wrote:
> > Using the prelude is customary in the kernel crate.
> >
> > Signed-off-by: Tamir Duberstein <tamird@gmail.com>
> > ---
> >  rust/kernel/xarray.rs | 34 ++++++++++++++++++++--------------
> >  1 file changed, 20 insertions(+), 14 deletions(-)
> >
> > diff --git a/rust/kernel/xarray.rs b/rust/kernel/xarray.rs
> > index 75719e7bb491..436faad99c89 100644
> > --- a/rust/kernel/xarray.rs
> > +++ b/rust/kernel/xarray.rs
> > @@ -5,16 +5,15 @@
> >  //! C header: [`include/linux/xarray.h`](srctree/include/linux/xarray.h)
> >
> >  use crate::{
> > -    alloc, bindings, build_assert,
> > -    error::{Error, Result},
> > +    alloc,
> > +    prelude::*,
> >      types::{ForeignOwnable, NotThreadSafe, Opaque},
> >  };
> > -use core::{iter, marker::PhantomData, mem, pin::Pin, ptr::NonNull};
> > -use pin_init::{pin_data, pin_init, pinned_drop, PinInit};
> > +use core::{iter, marker::PhantomData, mem, ptr::NonNull};
> >
> >  /// An array which efficiently maps sparse integer indices to owned objects.
> >  ///
> > -/// This is similar to a [`crate::alloc::kvec::Vec<Option<T>>`], but more efficient when there are
> > +/// This is similar to a [`Vec<Option<T>>`], but more efficient when there are
> >  /// holes in the index space, and can be efficiently grown.
> >  ///
> >  /// # Invariants
> > @@ -104,16 +103,23 @@ pub fn new(kind: AllocKind) -> impl PinInit<Self> {
> >      fn iter(&self) -> impl Iterator<Item = NonNull<T::PointedTo>> + '_ {
> >          let mut index = 0;
> >
> > -        // SAFETY: `self.xa` is always valid by the type invariant.
> > -        iter::once(unsafe {
> > -            bindings::xa_find(self.xa.get(), &mut index, usize::MAX, bindings::XA_PRESENT)
> > -        })
> > -        .chain(iter::from_fn(move || {
> > +        core::iter::Iterator::chain(
>
> Does this part come from using the prelude? If not, either we need to
> split the patch or we need to mention it in the changelog at least.

Yes, it's from using the prelude - PinInit also has a chain method
that causes ambiguity here.

> Also since we `use core::iter` above, we can avoid the `core::` here.

Good point.
Re: [PATCH 1/3] rust: xarray: use the prelude
Posted by Boqun Feng 3 months, 1 week ago 
On Tue, Jul 01, 2025 at 12:36:20PM -0400, Tamir Duberstein wrote:
> On Tue, Jul 1, 2025 at 12:35 PM Boqun Feng <boqun.feng@gmail.com> wrote:
> >
> > On Tue, Jul 01, 2025 at 12:27:17PM -0400, Tamir Duberstein wrote:
> > > Using the prelude is customary in the kernel crate.
> > >
> > > Signed-off-by: Tamir Duberstein <tamird@gmail.com>
> > > ---
> > >  rust/kernel/xarray.rs | 34 ++++++++++++++++++++--------------
> > >  1 file changed, 20 insertions(+), 14 deletions(-)
> > >
> > > diff --git a/rust/kernel/xarray.rs b/rust/kernel/xarray.rs
> > > index 75719e7bb491..436faad99c89 100644
> > > --- a/rust/kernel/xarray.rs
> > > +++ b/rust/kernel/xarray.rs
> > > @@ -5,16 +5,15 @@
> > >  //! C header: [`include/linux/xarray.h`](srctree/include/linux/xarray.h)
> > >
> > >  use crate::{
> > > -    alloc, bindings, build_assert,
> > > -    error::{Error, Result},
> > > +    alloc,
> > > +    prelude::*,
> > >      types::{ForeignOwnable, NotThreadSafe, Opaque},
> > >  };
> > > -use core::{iter, marker::PhantomData, mem, pin::Pin, ptr::NonNull};
> > > -use pin_init::{pin_data, pin_init, pinned_drop, PinInit};
> > > +use core::{iter, marker::PhantomData, mem, ptr::NonNull};
> > >
> > >  /// An array which efficiently maps sparse integer indices to owned objects.
> > >  ///
> > > -/// This is similar to a [`crate::alloc::kvec::Vec<Option<T>>`], but more efficient when there are
> > > +/// This is similar to a [`Vec<Option<T>>`], but more efficient when there are
> > >  /// holes in the index space, and can be efficiently grown.
> > >  ///
> > >  /// # Invariants
> > > @@ -104,16 +103,23 @@ pub fn new(kind: AllocKind) -> impl PinInit<Self> {
> > >      fn iter(&self) -> impl Iterator<Item = NonNull<T::PointedTo>> + '_ {
> > >          let mut index = 0;
> > >
> > > -        // SAFETY: `self.xa` is always valid by the type invariant.
> > > -        iter::once(unsafe {
> > > -            bindings::xa_find(self.xa.get(), &mut index, usize::MAX, bindings::XA_PRESENT)
> > > -        })
> > > -        .chain(iter::from_fn(move || {
> > > +        core::iter::Iterator::chain(
> >
> > Does this part come from using the prelude? If not, either we need to
> > split the patch or we need to mention it in the changelog at least.
> 
> Yes, it's from using the prelude - PinInit also has a chain method
> that causes ambiguity here.

Maybe you can mention this in the change log as well. Like "Calling
iter::chain() with the associated function style to disambiguate with
PinInit::chain()". Make it easier to review (and remember) why.

Regards,
Boqun

> 
> > Also since we `use core::iter` above, we can avoid the `core::` here.
> 
> Good point.
>
Re: [PATCH 1/3] rust: xarray: use the prelude
Posted by Tamir Duberstein 3 months, 1 week ago 
On Tue, Jul 1, 2025 at 1:02 PM Boqun Feng <boqun.feng@gmail.com> wrote:
>
> On Tue, Jul 01, 2025 at 12:36:20PM -0400, Tamir Duberstein wrote:
> > On Tue, Jul 1, 2025 at 12:35 PM Boqun Feng <boqun.feng@gmail.com> wrote:
> > >
> > > On Tue, Jul 01, 2025 at 12:27:17PM -0400, Tamir Duberstein wrote:
> > > > Using the prelude is customary in the kernel crate.
> > > >
> > > > Signed-off-by: Tamir Duberstein <tamird@gmail.com>
> > > > ---
> > > >  rust/kernel/xarray.rs | 34 ++++++++++++++++++++--------------
> > > >  1 file changed, 20 insertions(+), 14 deletions(-)
> > > >
> > > > diff --git a/rust/kernel/xarray.rs b/rust/kernel/xarray.rs
> > > > index 75719e7bb491..436faad99c89 100644
> > > > --- a/rust/kernel/xarray.rs
> > > > +++ b/rust/kernel/xarray.rs
> > > > @@ -5,16 +5,15 @@
> > > >  //! C header: [`include/linux/xarray.h`](srctree/include/linux/xarray.h)
> > > >
> > > >  use crate::{
> > > > -    alloc, bindings, build_assert,
> > > > -    error::{Error, Result},
> > > > +    alloc,
> > > > +    prelude::*,
> > > >      types::{ForeignOwnable, NotThreadSafe, Opaque},
> > > >  };
> > > > -use core::{iter, marker::PhantomData, mem, pin::Pin, ptr::NonNull};
> > > > -use pin_init::{pin_data, pin_init, pinned_drop, PinInit};
> > > > +use core::{iter, marker::PhantomData, mem, ptr::NonNull};
> > > >
> > > >  /// An array which efficiently maps sparse integer indices to owned objects.
> > > >  ///
> > > > -/// This is similar to a [`crate::alloc::kvec::Vec<Option<T>>`], but more efficient when there are
> > > > +/// This is similar to a [`Vec<Option<T>>`], but more efficient when there are
> > > >  /// holes in the index space, and can be efficiently grown.
> > > >  ///
> > > >  /// # Invariants
> > > > @@ -104,16 +103,23 @@ pub fn new(kind: AllocKind) -> impl PinInit<Self> {
> > > >      fn iter(&self) -> impl Iterator<Item = NonNull<T::PointedTo>> + '_ {
> > > >          let mut index = 0;
> > > >
> > > > -        // SAFETY: `self.xa` is always valid by the type invariant.
> > > > -        iter::once(unsafe {
> > > > -            bindings::xa_find(self.xa.get(), &mut index, usize::MAX, bindings::XA_PRESENT)
> > > > -        })
> > > > -        .chain(iter::from_fn(move || {
> > > > +        core::iter::Iterator::chain(
> > >
> > > Does this part come from using the prelude? If not, either we need to
> > > split the patch or we need to mention it in the changelog at least.
> >
> > Yes, it's from using the prelude - PinInit also has a chain method
> > that causes ambiguity here.
>
> Maybe you can mention this in the change log as well. Like "Calling
> iter::chain() with the associated function style to disambiguate with
> PinInit::chain()". Make it easier to review (and remember) why.

Yep, done for the next spin.

Patchew 2.1-devel-b67e4c2

© 2016 - 2025 Red Hat, Inc.