Re: [PATCH v2 17/21] rust: num: Add an upward alignment helper for usize

["Alexandre Courbot" <acourbot@nvidia.com>]

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 split 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 advised to take more inspiration
from them in order to avoid reinventing the wheel.

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.

> 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.