Message-Id: <DDXYZ5ZHVYCB.3F3GTUPLUQOAM@nvidia.com>
Date: Sun, 02 Nov 2025 14:46:01 +0900
From: "Alexandre Courbot" <acourbot@...dia.com>
To: "Alexandre Courbot" <acourbot@...dia.com>, "Alice Ryhl"
 <aliceryhl@...gle.com>, "Danilo Krummrich" <dakr@...nel.org>, "Miguel
 Ojeda" <ojeda@...nel.org>, "Joel Fernandes" <joelagnelf@...dia.com>, "Yury
 Norov" <yury.norov@...il.com>, "Jesung Yang" <y.j3ms.n@...il.com>, "Boqun
 Feng" <boqun.feng@...il.com>, "Gary Guo" <gary@...yguo.net>,
 Björn Roy Baron <bjorn3_gh@...tonmail.com>, "Benno Lossin"
 <lossin@...nel.org>, "Andreas Hindborg" <a.hindborg@...nel.org>, "Trevor
 Gross" <tmgross@...ch.edu>
Cc: <linux-kernel@...r.kernel.org>, <rust-for-linux@...r.kernel.org>
Subject: Re: [PATCH 1/2] rust: add BitInt integer wrapping type

Quick self-review as I've found a couple of points that can be improved.

On Fri Oct 31, 2025 at 10:39 PM JST, Alexandre Courbot wrote:
<snip>
> diff --git a/rust/kernel/num/bitint.rs b/rust/kernel/num/bitint.rs
> new file mode 100644
> index 000000000000..5af1447347a8
> --- /dev/null
> +++ b/rust/kernel/num/bitint.rs
> @@ -0,0 +1,896 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +//! [`BitInt`], a primitive integer type with a limited set of bits usable to represent values.
> +
> +use core::ops::Deref;
> +
> +use kernel::num::{Integer, Signed, Unsigned};
> +use kernel::prelude::*;
> +
> +/// Evaluates to `true` if `$value` can be represented using at most `$num_bits` on `$type`.
> +///
> +/// Can be used in const context.
> +macro_rules! fits_within {
> +    ($value:expr, $type:ty, $num_bits:expr) => {{
> +        // Any attempt to create a `BitInt` with more bits used for representation than its backing
> +        // type (i.e. create an invalid `BitInt`) must be aborted at build time.
> +        build_assert!(
> +            <$type>::BITS >= $num_bits,
> +            "Number of bits requested for representation is larger than backing type."
> +        );

All these similar invariant checks sprinkled around the code are very
error-prone. A better way to enforce them is to have a private
constructor that is called by any path that creates a new `BitInt`, and
have these invariants enforced in a const block:

    const fn __new(value: T) -> Self {
        // Enforce the type invariants.
        const {
            // `NUM_BITS` cannot be zero.
            build_assert!(NUM_BITS != 0);
            // The backing type is at least as large as `NUM_BITS`.
            build_assert!(NUM_BITS <= T::BITS);
        }

        Self(value)
    }

This means we do this in one place instead of 3 or 4, and since the
invariants are enforced in const context we get a decent error message
should they fail.

<snip>
> +mod atleast_impls {
> +    use super::*;
> +
> +    // Number of bits at least as large as `64`.
> +    impl_size_rule!(AtLeast64Bits, 64);
> +
> +    // Number of bits at least as large as `32`.
> +    impl_size_rule!(AtLeast32Bits,
> +        32 33 34 35 36 37 38 39
> +        40 41 42 43 44 45 46 47
> +        48 49 50 51 52 53 54 55
> +        56 57 58 59 60 61 62 63
> +    );
> +    // Anything larger than `64` is also larger than `32`.
> +    impl<T> AtLeast32Bits for T where T: AtLeast64Bits {}
> +
> +    // Number of bits at least as large as `16`.
> +    impl_size_rule!(AtLeast16Bits,
> +        16 17 18 19 20 21 22 23
> +        24 25 26 27 28 29 30 31
> +    );
> +    // Anything larger than `32` is also larger than `16`.
> +    impl<T> AtLeast16Bits for T where T: AtLeast32Bits {}
> +
> +    // Number of bits at least as large as a `8`.
> +    impl_size_rule!(AtLeast8Bits, 8 9 10 11 12 13 14 15);
> +    // Anything larger than `16` is also larger than `8`.
> +    impl<T> AtLeast8Bits for T where T: AtLeast16Bits {}
> +
> +    // Number of bits at least as large as `1`.
> +    impl_size_rule!(AtLeast1Bit, 1 2 3 4 5 6 7);
> +    // Anything larger than `8` is also larger than `1`.
> +    impl<T> AtLeast1Bit for T where T: AtLeast8Bits {}
> +
> +    /// Generates `From` implementations from a primitive type into a [`BitInt`] that is
> +    /// guaranteed to being able to contain it.
> +    macro_rules! impl_from_primitive {
> +        ($($type:ty => $trait:ident),*) => {
> +            $(
> +            impl<T, const NUM_BITS: u32> From<$type> for BitInt<T, NUM_BITS>
> +            where
> +                Self: $trait,
> +                T: From<$type> + Boundable,
> +            {
> +                fn from(value: $type) -> Self {
> +                    build_assert!(
> +                        T::BITS >= NUM_BITS,
> +                        "Number of bits requested for representation is larger than backing type."
> +                    );
> +                    // INVARIANT: the impl constraints guarantee that the source type is smaller
> +                    // than `NUM_BITS`, and the `build_assert` above that the backing type can
> +                    // contain `NUM_BITS`.
> +                    Self(T::from(value))
> +                }
> +            }
> +            )*
> +        }
> +    }
> +
> +    impl_from_primitive!(
> +        bool => AtLeast1Bit,
> +        u8 => AtLeast8Bits,
> +        i8 => AtLeast8Bits,
> +        u16 => AtLeast16Bits,
> +        i16 => AtLeast16Bits,
> +        u32 => AtLeast32Bits,
> +        i32 => AtLeast32Bits,
> +        u64 =>AtLeast64Bits,
> +        i64 =>AtLeast64Bits
> +    );

I dislike this part so much, especially since it could be done much more
cleanly if only we had `generic_const_exprs`. But for now we have to
work our way around with macros.

Still, this can be simplified significantly by replacing the
`AtLeast8Bits`, `AtLeast16Bits`, ... traits with a single
`AtLeastXBits<const N: usize>`.

By doing so, the cryptic

    Self: $trait,

constraint can become

    Self: AtLeastXBits<{ <$type as Integer>::BITS as usize }>,

which is much more explicit about its purpose, and we can remove the
`$trait` argument from the macro.

what more, we can even write a constraint like 

    Self: AtLeastXBits<{ <$type as Integer>::BITS as usize + 1 }>,

And support conversion from unsigned primitive types to signed `BitInt`s
with at least one more bit.

The same simplification can also be done for the `IntoXX` traits below.

I'll submit a v2 soonish with these improvements and some more minor
fixes.

Powered by blists - more mailing lists