Re: [PATCH v2 1/6] math.h: Add macros for rounding to the closest value

[Jani Nikula <jani.nikula@intel.com>]

On Tue, 27 Aug 2024, Devarsh Thakkar <devarsht@ti.com> wrote:
> Hi Nikula,
>
> Thanks for the review.
>
> On 27/08/24 18:10, Jani Nikula wrote:
>> On Tue, 27 Aug 2024, Devarsh Thakkar <devarsht@ti.com> wrote:
>
> [..]
>
>>> The equivalency between roundclosest w.r.t round_closest is same as
>>> equivalency between existing macros rounddown w.r.t round_down. Functionally
>>> both are same but the former is recommended to be used only for the scenario
>>> where multiple is not power of 2 and latter is faster but is strictly for the
>>> scenario where multiple is power of 2. I think the same is already summarized
>>> well in commit message and further elaborated in the patch itself as part of
>>> header file comments [1] so I personally don't think any update is required
>>> w.r.t this.
>> 
>> I still don't think rounddown vs. round_down naming is a good example to
>> model anything after.
>> 
>> I have yet to hear a single compelling argument in favor of having a
>> single underscore in the middle of a name having implications about the
>> inputs of a macro/function.
>> 
>> The macros being added here are at 2 or 3 in Rusty's scale [1]. We could
>> aim for 6 (The name tells you how to use it), but also do opportunistic
>> 8 (The compiler will warn if you get it wrong) for compile-time
>> constants.
>> 
>
> The macros use existing round_up/round_down underneath, so need to check if
> they have compile-time checks but either-way this should not block the current
> series as the series does not aim to enhance the existing round_up/round_down
> macros.
>
>> As-is, these, and round_up/round_down, are just setting a trap for an
>> unsuspecting kernel developer to fall into.
>> 
>
> I understand what you are saying but I believe this was already discussed in
> original patch series [1] where you had raised the same issue and it was even
> discussed if we want to go with a new naming convention (like
> round_closest_up_pow_2) [2] or not and the final consensus reached on naming
> was to align with the existing round*() macros [3]. Moreover, I didn't hear
> back any further arguments on this in further 8 revisions carrying this patch,
> so thought this was aligned per wider consensus.
>
> Anyways, to re-iterate the discussion do we want to change to below naming scheme?
>
> round_closest_up_pow_2
> round_closest_down_pow_2
> roundclosest
>
> or any other suggestions for naming ?
>
> If there is a wider consensus on the suggested name (and ok to deviate from
> existing naming convention), then we can go ahead with that.

The stupid thing here is, I still don't remember which one is the
generic thing, rounddown() or round_down(). I have to look it up every
single time to be sure. I refuse to believe I'd be the only one.

It's okay to accidentally use the generic version, no harm done. It's
definitely not okay to accidentally use the special pow-2 version, so it
should have a special name. I think _pow2() or _pow_2() is a fine
suffix.

Another stupid thing is, I'd name the generic thing round_down(). But
whoopsie, that's not the generic thing.

This naming puts an unnecessary extra burden on people. It's a stupid
"convention" to follow. It's a mistake. Not repeating a mistake trumps
following the pattern.

There, I've said it. Up to the people who ack and commit to decide.


BR,
Jani.


>
> [1]: https://lore.kernel.org/all/ZkIG0-01pz632l4R@smile.fi.intel.com/
> [2]: https://lore.kernel.org/all/5ebcf480-81c6-4c2d-96e8-727d44f21ca9@ti.com/
> [3]:
> https://lore.kernel.org/all/ZkIG0-01pz632l4R@smile.fi.intel.com/#:~:text=series%20is%20that%3A%0A%2D-,align,-naming%20(with%20the
>
> Regards
> Devarsh
>> 
>> BR,
>> Jani.
>> 
>> 
>> [1] https://ozlabs.org/~rusty/index.cgi/tech/2008-03-30.html
>> 
>> 
>>>
>>> [1]: https://lore.kernel.org/all/20240826150822.4057164-2-devarsht@ti.com
>>>
>>> Regards
>>> Devarsh
>> 

-- 
Jani Nikula, Intel