Nouveau - May 2025 - [PATCH v2 17/21] rust: num: Add an upward alignment helper for usize

On Sat May 3, 2025 at 12:02 PM JST, Joel Fernandes
wrote:
>
>
> On 5/2/2025 9:59 PM, Alexandre Courbot wrote:
>>> pub trait AlignUp {
>>>     fn align_up(self, alignment: Self) -> Self;
>>> }
>>>
>>> macro_rules! align_up_impl {
>>>     ($($t:ty),+) => {
>>>         $(
>>>             impl AlignUp for $t {
>>>                 fn align_up(self, alignment: Self) -> Self {
>>>                     (self + alignment - 1) & !(alignment - 1)
>>>                 }
>>>             }
>>>         )+
>>>     }
>>> }
>>>
>>> align_up_impl!(usize, u8, u16, u32, u64, u128);
>>>
>>> Or, we can even combine the 2 approaches. Use macros for the
"impl Alignable"
>>> and use generics on the Alignable trait.
>>>
>>> macro_rules! impl_alignable {
>>>     ($($t:ty),+) => {
>>>         $(
>>>             impl Alignable for $t {}
>>>         )+
>>>     };
>>> }
>>>
>>> impl_alignable!(usize, u8, u16, u32, u64, u128);
>>>
>>> pub trait AlignUp {
>>>     fn align_up(self, alignment: Self) -> Self;
>>> }
>>>
>>> impl<T> AlignUp for T
>>> where
>>>     T: Alignable,
>>> {
>>>     fn align_up(self, alignment: Self) -> Self {
>>>         let one = T::from(1u8);
>>>         (self + alignment - one) & !(alignment - one)
>>>     }
>>> }
>>>
>>> Thoughts?
>> I think that's the correct way to do it and am fully on board with
this
>> approach.
>> 
>> The only thing this doesn't solve is that it doesn't provide
`const`
>> functions. But maybe for that purpose we can use a single macro that
>> nicely panics at build-time should an overflow occur.
>
> Great, thanks. I split the traits as follows and it is cleaner and works. I
will
> look into the build-time overflow check and the returning of Result change
on
> Monday. Let me know if any objections.
Looking good IMHO, apart maybe from the names of the `BitOps` and
`Unsigned` traits that are not super descriptive and don't need to be
split for the moment anyway.

Actually it may be a good idea to move this into its own patch/series so
it gets more attention as this is starting to look like the `num` or
`num_integer` crates and we might be well-advised to take more
inspiration from them in order to avoid reinventing the wheel. It is
basically asking the question "how do we want to extend the integer
types in a useful way for the kernel", so it's actually pretty
important
that we get our answer right. :)

To address our immediate needs of an `align_up`, it just occurred to me
that we could simply use the `next_multiple_of` method, at least
temporarily. It is implemented with a modulo and will therefore probably
result in less efficient code than a version optimized for powers of
two, but it will do the trick until we figure out how we want to extend
the primitive types for the kernel, which is really what this patch is
about - we will also need an `align_down` for instance, and I don't know
of a standard library equivalent for it...
> I added the #[inline] and hopefully that
> gives similar benefits to const that you're seeking:
A `const` version is still going to be needed, `#[inline]` encourages the
compiler to try and inline the function, but AFAIK it doesn't allow use
in const context.