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