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

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

On Sat, Jun 14, 2025 at 08:56:36AM -0700, Boqun Feng wrote:
> 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.
> 

Ignore this, seems these functions meant to be not const, just the
document is wrong.

Regards,
Boqun

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