On Tue May 6, 2025 at 12:25 AM JST, Joel Fernandes wrote:
>> 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. :)
>
> I am not sure if we want to split the series for a simple change like this,
> because then the whole series gets blocked? It may also be better to pair the
> user of the function with the function itself IMHO since the function is also
> quite small. I am also Ok with keeping the original patch in the series and
> extending on that in the future (with just usize) to not block the series.
>
> Regarding for the full blown num module, I looked over the weekend and its
> actually a bunch of modules working together, with dozens of numeric APIs, so
> I
> am not sure if we should pull everything or try to copy parts of it. The R4l
> guidelines have something to say here. A good approach IMO is to just do it
> incrementally, like I'm doing with this patch.
>
> I think defining a "Unsigned" trait does make sense, and then for future
> expansion, it can be expanded on in the new num module?
Yeah maybe I was looking too far ahead. This can definitely grow
gradually.
>> 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...
>
> Why do we want to trade off for "less efficient code"? :) I think that's worse
> than the original change (before this series) I had which had no function call
> at all, but hardcoded the expression at the call site. The suggestion is also
> less desirable than having a local helper in the vbios module itself. I am not
> much a fan of the idea "lets call this temporarily and have sub optimal code"
> when the alternative is to just do it in-place, in-module, or via a num module
> extension :)
`next_multiple_of` has the benefit of returning the correct result even
for non-powers of 2, but at the same time trying to align to something
that is not a power of 2 is probably a defect in the code itself. ^_^;
Another reason for not using it is to have things properly named, so
agreed that an extension trait with the functionality we need, with a
name that clearly carries our intent and implemented as efficiently as
the C equivalent is better than reusing standard library methods that
happen to provide the correct result.
>>> 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.
>
> Right, so for the vbios use case there is no use of a const function. The only
> reason I added it is because there were other functions at the time which were
> used (by the now dropped timer module). I suggest let us add the const
> function
> once there is a user of it, I also don't know right how to do it. Like if I
> use
> generics for the const fn, I get this:
>
> const fn align_up_unsigned<T: Unsigned>(value: T, alignment: T) -> T {
> let one = T::from(1u8);
> (value + alignment - one) & !(alignment - one)
> }
>
> error[E0658]: cannot call conditionally-const method `<T as Add>::add` in
> constant functions
Interesting, I would expect that to fail but "conditionally-const"?
After looking that up is appears we can constraint a generic type
against a const trait, but that feature is still experimental and not
enabled in the kernel. So agreed, let's consider that later.
