Re: [PATCH 1/3] rust: add `num` module with `PowerOfTwo` type

["Alexandre Courbot" <acourbot@nvidia.com>]

Hi Benno,

Sorry, took some time to come back to this!

On Sun Jun 22, 2025 at 5:11 PM JST, Benno Lossin wrote:
> On Fri Jun 20, 2025 at 3:14 PM CEST, Alexandre Courbot wrote:
>> +/// An unsigned integer which is guaranteed to be a power of 2.
>> +///
>> +/// # Invariants
>> +///
>> +/// The stored value is guaranteed to be a power of two.
>> +#[derive(Debug, Clone, Copy, PartialEq, Eq, PartialOrd, Ord, Hash)]
>> +#[repr(transparent)]
>> +pub struct PowerOfTwo<T>(T);
>> +
>> +macro_rules! power_of_two_impl {
>> +    ($($t:ty),+) => {
>> +        $(
>> +            impl PowerOfTwo<$t> {
>
> I tried to use this type in a doctest like this:
>
>     use kernel::num::PowerOfTwo;
>    
>     fn new(x: usize) -> PowerOfTwo<usize> {
>         PowerOfTwo::new(1 << x)
>     }
>
> And it doesn't compile :(
>
>     error[E0034]: multiple applicable items in scope
>         --> rust/doctests_kernel_generated.rs:4930:17
>          |
>     4930 |     PowerOfTwo::new(1 << x)
>          |                 ^^^ multiple `new` found
>          |
>          = note: candidate #1 is defined in an impl for the type `PowerOfTwo<u128>`
>          = note: candidate #2 is defined in an impl for the type `PowerOfTwo<u16>`
>          = note: candidate #3 is defined in an impl for the type `PowerOfTwo<u32>`
>          = note: candidate #4 is defined in an impl for the type `PowerOfTwo<u64>`
>          = note: and 2 others
>     
>     error: aborting due to 1 previous error
>
> The problem is that the function `new` exists 6 times for each of the
> integer types. You can write `PowerOfTwo::<usize>::new()` instead, but
> that's annoying...

This should go away as we switch to the non-generic `Alignment` type
thankfully.

>
> We probably need an `Integer` trait and then do
>
>     impl<I: Integer> PowerOfTwo<I> {
>         pub const fn new(value: I) -> Self;
>     }
>
>> +                /// Validates that `v` is a power of two at build-time, and returns it wrapped into
>> +                /// [`PowerOfTwo`].
>> +                ///
>> +                /// A build error is triggered if `v` cannot be asserted to be a power of two.
>> +                ///
>> +                /// # Examples
>> +                ///
>> +                /// ```
>> +                /// use kernel::num::PowerOfTwo;
>> +                ///
>> +                #[doc = concat!("let v = PowerOfTwo::<", stringify!($t), ">::new(16);")]
>> +                /// assert_eq!(v.value(), 16);
>> +                /// ```
>> +                #[inline(always)]
>> +                pub const fn new(v: $t) -> Self {
>> +                    build_assert!(v.count_ones() == 1);
>
> Why not `v.is_power_of_two()`?

Why not indeed. :) Fixed.

>
>> +                    Self(v)
>
> Missing `// INVARIANT` comment.

Added (and in other places as well).

>
>> +                }
>> +
>> +                /// Validates that `v` is a power of two at runtime, and returns it wrapped into
>> +                /// [`PowerOfTwo`].
>> +                ///
>> +                /// [`None`] is returned if `v` was not a power of two.
>> +                ///
>> +                /// # Examples
>> +                ///
>> +                /// ```
>> +                /// use kernel::num::PowerOfTwo;
>> +                ///
>> +                #[doc = concat!(
>> +                    "assert_eq!(PowerOfTwo::<",
>> +                    stringify!($t),
>> +                    ">::try_new(16), Some(PowerOfTwo::<",
>> +                    stringify!($t),
>> +                    ">::new(16)));"
>> +                )]
>> +                #[doc = concat!(
>> +                    "assert_eq!(PowerOfTwo::<",
>> +                    stringify!($t),
>> +                    ">::try_new(15), None);"
>> +                )]
>> +                /// ```
>> +                #[inline(always)]
>> +                pub const fn try_new(v: $t) -> Option<Self> {
>
> Maybe `new_checked` is a better name, since it doesn't return a result?

Definitely.

>
>> +                    match v.count_ones() {
>
> Why not `is_power_of_two()`?

Fixed, thanks.

>
>> +                        1 => Some(Self(v)),
>
> Missing `// INVARIANT` comment.
>
>> +                        _ => None,
>> +                    }
>> +                }
>> +
>> +                /// Returns the value of this instance.
>> +                ///
>> +                /// It is guaranteed to be a power of two.
>> +                ///
>> +                /// # Examples
>> +                ///
>> +                /// ```
>> +                /// use kernel::num::PowerOfTwo;
>> +                ///
>> +                #[doc = concat!("let v = PowerOfTwo::<", stringify!($t), ">::new(16);")]
>> +                /// assert_eq!(v.value(), 16);
>> +                /// ```
>> +                #[inline(always)]
>> +                pub const fn value(self) -> $t {
>> +                    self.0
>
> Let's add:
>
>     if !self.0.is_power_of_two() {
>         core::hint::unreachable_unchecked()
>     }
>     self.0

Sure. Is it to enable compiler optimizations by making assumptions about
the returned value?

>
>> +                }
>> +
>> +                /// Returns the mask corresponding to `self.value() - 1`.
>> +                ///
>> +                /// # Examples
>> +                ///
>> +                /// ```
>> +                /// use kernel::num::PowerOfTwo;
>> +                ///
>> +                #[doc = concat!("let v = PowerOfTwo::<", stringify!($t), ">::new(0x10);")]
>> +                /// assert_eq!(v.mask(), 0xf);
>> +                /// ```
>> +                #[inline(always)]
>> +                pub const fn mask(self) -> $t {
>> +                    self.0.wrapping_sub(1)
>
> Then use `self.value().wrapping_sub(1)` here instead to also propagate
> the information.

Ack.