On Tue, 27 Aug 2024, Devarsh Thakkar <devarsht@xxxxxx> wrote: > Hi Nikula, > > Thanks for the review. > > On 27/08/24 18:10, Jani Nikula wrote: >> On Tue, 27 Aug 2024, Devarsh Thakkar <devarsht@xxxxxx> 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@xxxxxxxxxxxxxxxxxx/ > [2]: https://lore.kernel.org/all/5ebcf480-81c6-4c2d-96e8-727d44f21ca9@xxxxxx/ > [3]: > https://lore.kernel.org/all/ZkIG0-01pz632l4R@xxxxxxxxxxxxxxxxxx/#:~: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@xxxxxx >>> >>> Regards >>> Devarsh >> -- Jani Nikula, Intel
