Re: [PATCH 0/3] lib/string_helpers: Add a few string helpers

On Wed, 19 Jan 2022, Petr Mladek [off-list ref] wrote:

On Tue 2022-01-18 23:24:47, Lucas De Marchi wrote:

quoted

Add some helpers under lib/string_helpers.h so they can be used
throughout the kernel. When I started doing this there were 2 other
previous attempts I know of, not counting the iterations each of them
had:

1) https://lore.kernel.org/all/20191023131308.9420-1-jani.nikula@intel.com/ (local)
2) https://lore.kernel.org/all/20210215142137.64476-1-andriy.shevchenko@linux.intel.com/#t (local)

Going through the comments I tried to find some common ground and
justification for what is in here, addressing some of the concerns
raised.

d. This doesn't bring onoff() helper as there are some places in the
   kernel with onoff as variable - another name is probably needed for
   this function in order not to shadow the variable, or those variables
   could be renamed.  Or if people wanting  <someprefix>
   try to find a short one

I would call it str_on_off().

And I would actually suggest to use the same style also for
the other helpers.

The "str_" prefix would make it clear that it is something with
string. There are other <prefix>_on_off() that affect some
functionality, e.g. mute_led_on_off(), e1000_vlan_filter_on_off().

The dash '_' would significantly help to parse the name. yesno() and
onoff() are nicely short and kind of acceptable. But "enabledisable()"
is a puzzle.

IMHO, str_yes_no(), str_on_off(), str_enable_disable() are a good
compromise.

The main motivation should be code readability. You write the
code once. But many people will read it many times. Open coding
is sometimes better than misleading macro names.

That said, I do not want to block this patchset. If others like
it... ;-)

I don't mind the names either way. Adding the prefix and dashes is
helpful in that it's possible to add the functions first and convert
users at leisure, though with a bunch of churn, while using names that
collide with existing ones requires the changes to happen in one go.

What I do mind is grinding this series to a halt once again. I sent a
handful of versions of this three years ago, with inconclusive
bikeshedding back and forth, eventually threw my hands up in disgust,
and walked away.

quoted

e. One alternative to all of this suggested by Christian König
   (43456ba7-c372-84cc-4949-dcb817188e21@amd.com) would be to add a
   printk format. But besides the comment, he also seemed to like
   the common function. This brought the argument from others that the
   simple yesno()/enabledisable() already used in the code is easier to
   remember and use than e.g. %py[DOY]

Thanks for not going this way :-)

quoted

Last patch also has some additional conversion of open coded cases. I
preferred starting with drm/ since this is "closer to home".

I hope this is a good summary of the previous attempts and a way we can
move forward.

Andrew Morton, Petr Mladek, Andy Shevchenko: if this is accepted, my
proposal is to take first 2 patches either through mm tree or maybe
vsprintf. Last patch can be taken later through drm.

I agree with Andy that it should go via drm tree. It would make it
easier to handle potential conflicts.

Just in case, you decide to go with str_yes_no() or something similar.
Mass changes are typically done at the end on the merge window.
The best solution is when it can be done by a script.

Best Regards,
Petr

-- 
Jani Nikula, Intel Open Source Graphics Center