Re: [PATCH v4] string-choice: add yesno(), onoff(), enableddisabled(), plural() helpers

[Jani Nikula]

On Wed, 23 Oct 2019, Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx> wrote:
> On Wed, 23 Oct 2019 16:13:08 +0300 Jani Nikula <jani.nikula@xxxxxxxxx> wrote:
>
>> The kernel has plenty of ternary operators to choose between constant
>> strings, such as condition ? "yes" : "no", as well as value == 1 ? "" :
>> "s":
>> 
>> $ git grep '? "yes" : "no"' | wc -l
>> 258
>> $ git grep '? "on" : "off"' | wc -l
>> 204
>> $ git grep '? "enabled" : "disabled"' | wc -l
>> 196
>> $ git grep '? "" : "s"' | wc -l
>> 25
>> 
>> Additionally, there are some occurences of the same in reverse order,
>> split to multiple lines, or otherwise not caught by the simple grep.
>> 
>> Add helpers to return the constant strings. Remove existing equivalent
>> and conflicting functions in i915, cxgb4, and USB core. Further
>> conversion can be done incrementally.
>> 
>> The main goal here is to abstract recurring patterns, and slightly clean
>> up the code base by not open coding the ternary operators.
>
> Fair enough.
>
>> --- /dev/null
>> +++ b/include/linux/string-choice.h
>> @@ -0,0 +1,31 @@
>> +/* SPDX-License-Identifier: MIT */
>> +/*
>> + * Copyright  2019 Intel Corporation
>> + */
>> +
>> +#ifndef __STRING_CHOICE_H__
>> +#define __STRING_CHOICE_H__
>> +
>> +#include <linux/types.h>
>> +
>> +static inline const char *yesno(bool v)
>> +{
>> +	return v ? "yes" : "no";
>> +}
>> +
>> +static inline const char *onoff(bool v)
>> +{
>> +	return v ? "on" : "off";
>> +}
>> +
>> +static inline const char *enableddisabled(bool v)
>> +{
>> +	return v ? "enabled" : "disabled";
>> +}
>> +
>> +static inline const char *plural(long v)
>> +{
>> +	return v == 1 ? "" : "s";
>> +}
>> +
>> +#endif /* __STRING_CHOICE_H__ */
>
> These aren't very good function names.  Better to create a kernel-style
> namespace such as "choice_" and then add the expected underscores:
>
> choice_yes_no()
> choice_enabled_disabled()
> choice_plural()

I was merely using existing function names used in several drivers in
the kernel. But I can rename no problem.

Are your suggestions the names we can settle on now, or should I expect
to receive more opinions, but only after I send v5?

> (Example: note that slabinfo.c already has an "onoff()").

Under tools/ though? I did mean to address all conflicts in this patch.

> Also, I worry that making these functions inline means that each .o
> file will contain its own copy of the strings ("yes", "no", "enabled",
> etc) if the .c file calls the relevant helper.  I'm not sure if the
> linker is smart enough (yet) to fix this up.  If not, we will end up
> with a smaller kernel by uninlining these functions. 
> lib/string-choice.c would suit.
>
> And doing this will cause additional savings: calling a single-arg
> out-of-line function generates less .text than calling yesno().  When I
> did this: 
>
> --- a/include/linux/string-choice.h~string-choice-add-yesno-onoff-enableddisabled-plural-helpers-fix
> +++ a/include/linux/string-choice.h
> @@ -8,10 +8,7 @@
>  
>  #include <linux/types.h>
>  
> -static inline const char *yesno(bool v)
> -{
> -	return v ? "yes" : "no";
> -}
> +const char *yesno(bool v);
>  
>  static inline const char *onoff(bool v)
>  {
>
> The text segment of drivers/net/ethernet/chelsio/cxgb4/cxgb4_debugfs.o
> (78 callsites) shrunk by 118 bytes.

So we've already been back and forth on that particular topic in the
history of this patch. v2 had lib/string-choice.c and no inlines [1].

In the end, starting to use functions, inline or not, will let us rework
the implementation as we see fit, without touching the callers.

Again, it's no problem to go back to lib/string-choice.c, *once* more,
and the effort is trivial, but the ping-pong is getting old.


BR,
Jani.


[1] http://lore.kernel.org/r/20190930141842.15075-1-jani.nikula@xxxxxxxxx

-- 
Jani Nikula, Intel Open Source Graphics Center