On 10/10/2025 4:49 AM, Alexandre Courbot wrote:
> On Fri Oct 10, 2025 at 6:33 AM JST, Joel Fernandes wrote:
>> Hi Alex,
>>
>> Great effort, thanks. I replied with few comments below. Since the patch is
>> large, it would be great if could be possibly split. Maybe the From
>> primitives deserve a separate patch.
>
> I'm all for smaller patches when it makes reviewing easier, but in this
> case all it would achieve is making the second patch append code right
> after the next. :) Is there a benefit in doing so?
I think there is a benefit, because reviewers don't have to scroll through huge
emails :). Plus separate commit messages would be added in too, to reason about
new code. There's other possible logical splits too, was just giving example but
I am Ok with it if you still want to keep it singular. :)
>>> +impl<const NUM_BITS: u32> Boundable<NUM_BITS> for u16 {
>>> + const MASK: u16 = crate::bits::genmask_u16(0..=(NUM_BITS - 1));
>>> +}
>>> +
>>> +impl<const NUM_BITS: u32> Boundable<NUM_BITS> for u32 {
>>> + const MASK: u32 = crate::bits::genmask_u32(0..=(NUM_BITS - 1));
>>> +}
>>> +
>>> +impl<const NUM_BITS: u32> Boundable<NUM_BITS> for u64 {
>>> + const MASK: u64 = crate::bits::genmask_u64(0..=(NUM_BITS - 1));
>>> +}
>>> +
>>> +impl<T, const NUM_BITS: u32> BoundedInt<T, NUM_BITS>
>>> +where
>>> + T: Boundable<NUM_BITS>,
>>> +{
>>> + /// Checks that `value` is valid for this type at compile-time and
>>> build a new value.
>>> + ///
>>> + /// This relies on [`build_assert!`] to perform validation at
>>> compile-time. If `value` cannot
>>> + /// be inferred to be in bounds at compile-time, use the fallible
>>> [`Self::try_new`] instead.
>>> + ///
>>> + /// When possible, use one of the `new_const` methods instead of this
>>> method as it statically
>>> + /// validates `value` instead of relying on the compiler's
>>> optimizations.
>>
>> This sounds like, users might use the less-optimal API first with the same
>> build_assert issues we had with the IO accessors, since new() sounds very
>> obvious.
>> How about the following naming?
>>
>> new::<VALUE>() // Primary constructor for constants using const
>> generics.
>> try_new(value) // Keep as-is for fallible runtime
>> new_from_expr(value) // For compile-time validated runtime values
>>
>> If new::<VALUE>() does not work for the user, the compiler will fail.
>>
>> Or, new_from_expr() could be from_value(), Ok with either naming or a better
>> name.
>
> Agreed, the preferred method should appear first. IIRC Alice also made a
> similar suggestion about v1 during the DRM sync, sorry for not picking
> it up.
Cool, sounds good.
[..]>>> + /// Returns the contained value as a primitive type.
>>> + ///
>>> + /// # Examples
>>> + ///
>>> + /// ```
>>> + /// use kernel::num::BoundedInt;
>>> + ///
>>> + /// let v = BoundedInt::<u32, 4>::new_const::<7>();
>>> + /// assert_eq!(v.get(), 7u32);
>>> + /// ```
>>> + pub fn get(self) -> T {
>>> + if !T::is_in_bounds(self.0) {
>>> + // SAFETY: Per the invariants, `self.0` cannot have bits set
>>> outside of `MASK`, so
>>> + // this block will
>>> + // never be reached.
>>> + unsafe { core::hint::unreachable_unchecked() }
>>> + }
>>
>> Does this if block help the compiler generate better code? I wonder if code
>> gen could be checked to confirm the rationale.
>
> Benno suggested that it would on a different patch:
>
> https://lore.kernel.org/rust-for-linux/dbl1zgzcsjf3.29hns9bsn8...@kernel.org/
>
> OTOH as shown in patch 3/3, this doesn't exempt us from handling
> impossible values when using this in a match expression...
Interesting, TIL.
>>> + self.0
>>> + }
>>> +
>>> + /// Increase the number of bits usable for `self`.
>>> + ///
>>> + /// This operation cannot fail.
>>> + ///
>>> + /// # Examples
>>> + ///
>>> + /// ```
>>> + /// use kernel::num::BoundedInt;
>>> + ///
>>> + /// let v = BoundedInt::<u32, 4>::new_const::<7>();
>>> + /// let larger_v = v.enlarge::<12>();
>>> + /// // The contained values are equal even though `larger_v` has a
>>> bigger capacity.
>>> + /// assert_eq!(larger_v, v);
>>> + /// ```
>>> + pub const fn enlarge<const NEW_NUM_BITS: u32>(self) -> BoundedInt<T,
>>> NEW_NUM_BITS>
>>> + where
>>> + T: Boundable<NEW_NUM_BITS>,
>>> + T: Copy,
>>
>> Boundable already implies copy so T: Copy is redundant.
>
> Thanks. I need to do a thorough review of all the contraints as I've
> changed them quite a bit as the implementation matured.
Cool. :)
>> [...]
>>> +impl_const_new!(u8 u16 u32 u64);
>>> +
>>> +/// Declares a new `$trait` and implements it for all bounded types
>>> represented using `$num_bits`.
>>> +///
>>> +/// This is used to declare properties as traits that we can use for later
>>> implementations.
>>> +macro_rules! impl_size_rule {
>>> + ($trait:ident, $($num_bits:literal)*) => {
>>> + trait $trait {}
>>> +
>>> + $(
>>> + impl<T> $trait for BoundedInt<T, $num_bits> where T:
>>> Boundable<$num_bits> {}
>>> + )*
>>> + };
>>> +}
>>> +
>>> +// Bounds that are larger than a `u64`.
>>> +impl_size_rule!(LargerThanU64, 64);
>>> +
>>> +// Bounds that are larger than a `u32`.
>>> +impl_size_rule!(LargerThanU32,
>>> + 32 33 34 35 36 37 38 39
>>
>> If num_bits == 32 (number of bits), how could BoundedInt<T, 32> a
>> LargerThanU32? It should be AtleastU32 or something.
>>
>> Or the above list should start from 33. Only a >= 33-bit wide integer can be
>> LargerThanU32.
>
> The name is a bit ambiguous indeed. An accurate one would be
> `LargerOrEqualThanU32`, but `AtLeastU32` should also work.
Sure, I prefer AtLeastU32 since it is shorter :)
thanks,
- Joel
