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

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

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
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,
> > +            }
[...]