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

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

On Thu, 20 Jan 2022, Petr Mladek <pmladek@suse.com> wrote:
> On Wed 2022-01-19 16:16:12, Jani Nikula wrote:
>> On Wed, 19 Jan 2022, Petr Mladek <pmladek@suse.com> wrote:
>> > On Tue 2022-01-18 23:24:47, Lucas De Marchi wrote:
>> >> 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.
>
>> 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