John Hubbard
2025-Jun-12 20:00 UTC
[PATCH v5 04/23] rust: add new `num` module with `PowerOfTwo` type
On 6/12/25 8:07 AM, Boqun Feng wrote:> On Thu, Jun 12, 2025 at 11:01:32PM +0900, Alexandre Courbot wrote:...>> + #[inline(always)] >> + pub const fn align_down(self, value: $t) -> $t { > > I'm late to party, but could we instead implement: > > pub const fn round_down<i32>(value: i32, shift: i32) -> i32 { > value & !((1 << shift) - 1) > } > > pub const fn round_up<i32>(value: i32, shift: i32) -> i32 { > let mask = (1 << shift) - 1; > value.wrapping_add(mask) & !mask > }Just a naming concern here. The function name, and the "shift" argument is extremely odd there. And that's because it is re-inventing the concept of align_down() and align_up(), but with a misleading name and a hard to understand "shift" argument. If you are "rounding" to a power of two, that's normally called alignment, at least in kernel code. And if you are rounding to the nearest...integer, for example, that's rounding. But "rounding" with a "shift" argument? That's a little too creative! :)> > ? It's much harder to pass an invalid alignment with this.Hopefully we can address argument validation without blowing up the usual naming conventions. thanks, -- John Hubbard
Boqun Feng
2025-Jun-12 20:05 UTC
[PATCH v5 04/23] rust: add new `num` module with `PowerOfTwo` type
On Thu, Jun 12, 2025 at 01:00:12PM -0700, John Hubbard wrote:> On 6/12/25 8:07 AM, Boqun Feng wrote: > > On Thu, Jun 12, 2025 at 11:01:32PM +0900, Alexandre Courbot wrote: > ... > >> + #[inline(always)] > >> + pub const fn align_down(self, value: $t) -> $t { > > > > I'm late to party, but could we instead implement: > > > > pub const fn round_down<i32>(value: i32, shift: i32) -> i32 { > > value & !((1 << shift) - 1) > > } > > > > pub const fn round_up<i32>(value: i32, shift: i32) -> i32 { > > let mask = (1 << shift) - 1; > > value.wrapping_add(mask) & !mask > > } > > Just a naming concern here. > > The function name, and the "shift" argument is extremely odd there. > And that's because it is re-inventing the concept of align_down() > and align_up(), but with a misleading name and a hard to understand > "shift" argument. > > If you are "rounding" to a power of two, that's normally called > alignment, at least in kernel code. And if you are rounding to the > nearest...integer, for example, that's rounding. > > But "rounding" with a "shift" argument? That's a little too > creative! :) >Oh, sorry, I should have mentioned where I got these names, see round_up() and round_down() in include/linux/math.h. But no objection to find a better name for "shift". Regards, Boqun> > > > ? It's much harder to pass an invalid alignment with this. > > Hopefully we can address argument validation without blowing up > the usual naming conventions. > > > thanks, > -- > John Hubbard >
John Hubbard
2025-Jun-12 20:08 UTC
[PATCH v5 04/23] rust: add new `num` module with `PowerOfTwo` type
On 6/12/25 1:05 PM, Boqun Feng wrote:> On Thu, Jun 12, 2025 at 01:00:12PM -0700, John Hubbard wrote: >> On 6/12/25 8:07 AM, Boqun Feng wrote: >>> On Thu, Jun 12, 2025 at 11:01:32PM +0900, Alexandre Courbot wrote: >> ... >>>> + #[inline(always)] >>>> + pub const fn align_down(self, value: $t) -> $t { >>> >>> I'm late to party, but could we instead implement: >>> >>> pub const fn round_down<i32>(value: i32, shift: i32) -> i32 { >>> value & !((1 << shift) - 1) >>> } >>> >>> pub const fn round_up<i32>(value: i32, shift: i32) -> i32 { >>> let mask = (1 << shift) - 1; >>> value.wrapping_add(mask) & !mask >>> } >> >> Just a naming concern here. >> >> The function name, and the "shift" argument is extremely odd there. >> And that's because it is re-inventing the concept of align_down() >> and align_up(), but with a misleading name and a hard to understand >> "shift" argument. >> >> If you are "rounding" to a power of two, that's normally called >> alignment, at least in kernel code. And if you are rounding to the >> nearest...integer, for example, that's rounding. >> >> But "rounding" with a "shift" argument? That's a little too >> creative! :) >> > > Oh, sorry, I should have mentioned where I got these names, see > round_up() and round_down() in include/linux/math.h. But no objection to > find a better name for "shift".lol, perfect response! So my complaint is really about the kernel's existing math.h, rather than your proposal. OK then. :) thanks, -- John Hubbard