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

On Thu, 20 Jan 2022, Petr Mladek [off-list ref] wrote:

On Wed 2022-01-19 16:16:12, Jani Nikula wrote:

quoted

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

quoted

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

quoted

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.

It is also possible to support both notations at the beginning.
And convert the existing users in the 2nd step.

quoted

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.

Yeah, and I am sorry for bikeshedding. Honestly, I do not know what is
better. This is why I do not want to block this series when others
like this.

My main motivation is to point out that:

    enabledisable(enable)

might be, for some people, more eye bleeding than

    enable ? "enable" : "disable"


The problem is not that visible with yesno() and onoff(). But as you said,
onoff() confliscts with variable names. And enabledisable() sucks.
As a result, there is a non-trivial risk of two mass changes:

My point is, in the past three years we could have churned through more
than two mass renames just fine, if needed, *if* we had just managed to
merge something for a start!

BR,
Jani.

now:

- contition ? "yes" : "no"
+ yesno(condition)

a few moths later:

- yesno(condition)
+ str_yes_no(condition)


Best Regards,
Petr

-- 
Jani Nikula, Intel Open Source Graphics Center