Nouveau - Jan 2022 - [Intel-gfx] [PATCH 0/3] lib/string_helpers: Add a few string helpers

On Wed, Jan 19, 2022 at 04:16:12PM +0200, Jani Nikula
wrote:
> On Wed, 19 Jan 2022, Petr Mladek <pmladek at suse.com> wrote:
> > On Tue 2022-01-18 23:24:47, Lucas De Marchi wrote:
> >> 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
at intel.com/
> >> 2)
https://lore.kernel.org/all/20210215142137.64476-1-andriy.shevchenko at
linux.intel.com/#t
> >> 
> >> 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.
Yeah we can sed this anytime later we want to, but we need to get the foot
in the door. There's also a pile more of these all over.

Acked-by: Daniel Vetter <daniel.vetter at ffwll.ch>

on the series, maybe it helps? And yes let's merge this through drm-misc.
-Daniel
> 
> >
> >
> >> e. One alternative to all of this suggested by Christian K?nig
> >>    (43456ba7-c372-84cc-4949-dcb817188e21 at 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 :-)
> >
> >> 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
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

On Wed, Jan 19, 2022 at 05:15:02PM +0100, Daniel Vetter
wrote:
>On Wed, Jan 19, 2022 at 04:16:12PM +0200, Jani Nikula wrote:
>> On Wed, 19 Jan 2022, Petr Mladek <pmladek at suse.com> wrote:
>> > On Tue 2022-01-18 23:24:47, Lucas De Marchi wrote:
>> >> 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 at intel.com/
>> >> 2)
https://lore.kernel.org/all/20210215142137.64476-1-andriy.shevchenko at
linux.intel.com/#t
>> >>
>> >> 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.
>
>Yeah we can sed this anytime later we want to, but we need to get the foot
>in the door. There's also a pile more of these all over.
>
>Acked-by: Daniel Vetter <daniel.vetter at ffwll.ch>
>
>on the series, maybe it helps? And yes let's merge this through
drm-misc.
Ok, it seems we are reaching some agreement here then:

- Change it to use str_ prefix
- Wait -rc1 to avoid conflict
- Merge through drm-misc

I will re-send the series again soon.

Lucas De Marchi

On Wed, Jan 19, 2022 at 12:53:43PM -0800, Lucas De Marchi
wrote:
> On Wed, Jan 19, 2022 at 05:15:02PM +0100, Daniel Vetter wrote:
> > On Wed, Jan 19, 2022 at 04:16:12PM +0200, Jani Nikula wrote:
...
> > Yeah we can sed this anytime later we want to, but we need to get the
foot
> > in the door. There's also a pile more of these all over.
> > 
> > Acked-by: Daniel Vetter <daniel.vetter at ffwll.ch>
> > 
> > on the series, maybe it helps? And yes let's merge this through
drm-misc.
> 
> Ok, it seems we are reaching some agreement here then:
> - Change it to use str_ prefix
Not sure about this (*), but have no strong argument against. Up to you.
Ah, yes, Jani said about additional churn this change will make if goes
together with this series. Perhaps it can be done separately?
> - Wait -rc1 to avoid conflict
> - Merge through drm-misc
*) E.g. yesno() to me doesn't sound too bad to misunderstand its meaning,
   esp.when it's used as an argument to the printf() functions.

-- 
With Best Regards,
Andy Shevchenko