Re: [PATCH v6] rust: kernel: add support for bits/genmask macros

[Boqun Feng <boqun.feng@gmail.com>]

On Sat, Jun 14, 2025 at 08:06:04AM -0700, Boqun Feng wrote:
> On Sat, Jun 14, 2025 at 10:38:11PM +0900, Alexandre Courbot wrote:
> [...]
> > > +macro_rules! impl_genmask_fn {
> > > +    (
> > > +        $ty:ty, $checked_bit:ident, $bit:ident, $genmask:ident, $genmask_checked:ident, $genmask_unbounded:ident,
> > > +        $(#[$genmask_ex:meta])*
> > > +    ) => {
> > > +        /// Creates a compile-time contiguous bitmask for the given range by
> > > +        /// validating the range at runtime.
> > > +        ///
> > > +        /// Returns [`None`] if the range is invalid, i.e.: if the start is
> > > +        /// greater than or equal to the end.
> > > +        #[inline]
> > > +        pub fn $genmask_checked(range: Range<u32>) -> Option<$ty> {
> > > +            if range.start >= range.end || range.end > <$ty>::BITS {
> > > +                return None;
> > > +            }
> > 
> > From this check I assumed that you interpret `range` as non-inclusive,
> > since `range.end == 32` is valid on u32...
> > 
> > > +            let high = $checked_bit(range.end)?;
> > 
> > ... however IIUC `checked_bit` will return `None` here in such a case.
> > Should the argument be `range.end - 1`?
> > 
> > Your examples do seem to interpret the range as inclusive though, so
> > probably the check should be `|| range.end >= <$ty>::BITS`. But that
> > triggers the question, is it ok to use `Range` that way, when its
> > documentation specifically states that it is bounded exclusively above?
> > We could use `RangeInclusive` to match the semantics, which would
> > require us to write the ranges as `0..=7`. At least it is clear that the
> > upper bound is inclusive.
> > 
> > ... or we make the methods generic against `RangeBounds` and allow both
> > `Range` and `RangeInclusive` to be used. But I'm concerned that callers
> > might use `0..1` thinking it is inclusive while it is not.
> > 
> 
> I think generic over `RangeBounds` is a good idea, and we should
> .is_emtpy() or .contains() instead of comparison + boolean operation
> when possible. Seems we need a function to check whether one range

Ah.. from the other email, genmask_checked() needs to be const, then I
think we cannot use RangeBounds here? Because RangeBounds::start() is 
not a const function.

Regards,
Boqun

> contains another range, which is not available currently?
> 
> I would not be worried about callers treating `0..1` as inclusive: this
> is a Rust project anyway, we need to learn the correct semantics of
> expressions eventually ;-)
> 
> Regards,
> Boqun
> 
> > Thoughts?
> > 
> > > +            let low = $checked_bit(range.start)?;
> > > +            Some((high | (high - 1)) & !(low - 1))
> > > +        }
> > > +
> > > +        /// Creates a compile-time contiguous bitmask for the given range by
> > > +        /// validating the range at runtime.
> > > +        ///
> > > +        /// Returns `0` if the range is invalid, i.e.: if the start is greater
> > > +        /// than or equal to the end.
> > > +        #[inline]
> > > +        pub fn $genmask_unbounded(range: Range<u32>) -> $ty {
> > > +            match $genmask_checked(range) {
> > > +                Some(v) => v,
> > > +                None => 0,
> > > +            }
> [...]