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

From: Andrew Morton <akpm@linux-foundation.org>
To: Jani Nikula <jani.nikula@intel.com>
Cc: linux-kernel@vger.kernel.org,
	Joonas Lahtinen <joonas.lahtinen@linux.intel.com>,
	Rodrigo Vivi <rodrigo.vivi@intel.com>,
	intel-gfx@lists.freedesktop.org,
	Vishal Kulkarni <vishal@chelsio.com>,
	netdev@vger.kernel.org,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	linux-usb@vger.kernel.org, Julia Lawall <julia.lawall@lip6.fr>,
	Rasmus Villemoes <linux@rasmusvillemoes.dk>
Subject: Re: [PATCH v4] string-choice: add yesno(), onoff(), enableddisabled(), plural() helpers
Date: Wed, 23 Oct 2019 15:56:19 -0700	[thread overview]
Message-ID: <20191023155619.43e0013f0c8c673a5c508c1e@linux-foundation.org> (raw)
In-Reply-To: <20191023131308.9420-1-jani.nikula@intel.com>

On Wed, 23 Oct 2019 16:13:08 +0300 Jani Nikula <jani.nikula@intel.com> 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()

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


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.

