[PATCH v2] lib/string-choice: add yesno(), onoff(), enableddisabled(), plural() helpers

[PATCH v2] lib/string-choice: add yesno(), onoff(), enableddisabled(), plural() helpers

[Jani Nikula]

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.

While the main goal here is to abstract recurring patterns, and slightly
clean up the code base by not open coding the ternary operators, there
are also some space savings to be had via better string constant
pooling.

Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Cc: intel-gfx@lists.freedesktop.org
Cc: Vishal Kulkarni <vishal@chelsio.com>
Cc: netdev@vger.kernel.org
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: linux-usb@vger.kernel.org
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: linux-kernel@vger.kernel.org
Cc: Julia Lawall <julia.lawall@lip6.fr>
Cc: Rasmus Villemoes <linux@rasmusvillemoes.dk>
Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> # v1
Signed-off-by: Jani Nikula <jani.nikula@intel.com>

---

v2: add string-choice.[ch] to not clutter kernel.h and to actually save
space on string constants.

Example of further cleanup possibilities are at [1], to be done
incrementally afterwards.

[1] http://lore.kernel.org/r/20190903133731.2094-2-jani.nikula@intel.com
---
 drivers/gpu/drm/i915/i915_utils.h             | 16 +---------
 .../ethernet/chelsio/cxgb4/cxgb4_debugfs.c    | 12 +------
 drivers/usb/core/config.c                     |  6 +---
 drivers/usb/core/generic.c                    |  6 +---
 include/linux/kernel.h                        |  1 +
 include/linux/string-choice.h                 | 16 ++++++++++
 lib/Makefile                                  |  2 +-
 lib/string-choice.c                           | 31 +++++++++++++++++++
 8 files changed, 53 insertions(+), 37 deletions(-)
 create mode 100644 include/linux/string-choice.h
 create mode 100644 lib/string-choice.c

diff --git a/drivers/gpu/drm/i915/i915_utils.h b/drivers/gpu/drm/i915/i915_utils.h
index 562f756da421..794f02a90efe 100644
--- a/drivers/gpu/drm/i915/i915_utils.h
+++ b/drivers/gpu/drm/i915/i915_utils.h
@@ -28,6 +28,7 @@
 #include <linux/list.h>
 #include <linux/overflow.h>
 #include <linux/sched.h>
+#include <linux/string-choice.h>
 #include <linux/types.h>
 #include <linux/workqueue.h>
 
@@ -395,21 +396,6 @@ wait_remaining_ms_from_jiffies(unsigned long timestamp_jiffies, int to_wait_ms)
 #define MBps(x) KBps(1000 * (x))
 #define GBps(x) ((u64)1000 * MBps((x)))
 
-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 void add_taint_for_CI(unsigned int taint)
 {
 	/*
diff --git a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_debugfs.c b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_debugfs.c
index ae6a47dd7dc9..d9123dae1d00 100644
--- a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_debugfs.c
+++ b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_debugfs.c
@@ -35,6 +35,7 @@
 #include <linux/seq_file.h>
 #include <linux/debugfs.h>
 #include <linux/string_helpers.h>
+#include <linux/string-choice.h>
 #include <linux/sort.h>
 #include <linux/ctype.h>
 
@@ -2023,17 +2024,6 @@ static const struct file_operations rss_debugfs_fops = {
 /* RSS Configuration.
  */
 
-/* Small utility function to return the strings "yes" or "no" if the supplied
- * argument is non-zero.
- */
-static const char *yesno(int x)
-{
-	static const char *yes = "yes";
-	static const char *no = "no";
-
-	return x ? yes : no;
-}
-
 static int rss_config_show(struct seq_file *seq, void *v)
 {
 	struct adapter *adapter = seq->private;
diff --git a/drivers/usb/core/config.c b/drivers/usb/core/config.c
index 151a74a54386..52cee9067eb4 100644
--- a/drivers/usb/core/config.c
+++ b/drivers/usb/core/config.c
@@ -10,6 +10,7 @@
 #include <linux/module.h>
 #include <linux/slab.h>
 #include <linux/device.h>
+#include <linux/string-choice.h>
 #include <asm/byteorder.h>
 #include "usb.h"
 
@@ -19,11 +20,6 @@
 #define USB_MAXCONFIG			8	/* Arbitrary limit */
 
 
-static inline const char *plural(int n)
-{
-	return (n == 1 ? "" : "s");
-}
-
 static int find_next_descriptor(unsigned char *buffer, int size,
     int dt1, int dt2, int *num_skipped)
 {
diff --git a/drivers/usb/core/generic.c b/drivers/usb/core/generic.c
index 38f8b3e31762..a784a09794d6 100644
--- a/drivers/usb/core/generic.c
+++ b/drivers/usb/core/generic.c
@@ -21,14 +21,10 @@
 
 #include <linux/usb.h>
 #include <linux/usb/hcd.h>
+#include <linux/string-choice.h>
 #include <uapi/linux/usb/audio.h>
 #include "usb.h"
 
-static inline const char *plural(int n)
-{
-	return (n == 1 ? "" : "s");
-}
-
 static int is_rndis(struct usb_interface_descriptor *desc)
 {
 	return desc->bInterfaceClass == USB_CLASS_COMM
diff --git a/include/linux/kernel.h b/include/linux/kernel.h
index d83d403dac2e..91ace0e6ec1d 100644
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h
@@ -1029,4 +1029,5 @@ static inline void ftrace_dump(enum ftrace_dump_mode oops_dump_mode) { }
 	 /* OTHER_WRITABLE?  Generally considered a bad idea. */		\
 	 BUILD_BUG_ON_ZERO((perms) & 2) +					\
 	 (perms))
+
 #endif
diff --git a/include/linux/string-choice.h b/include/linux/string-choice.h
new file mode 100644
index 000000000000..1583b909e2c9
--- /dev/null
+++ b/include/linux/string-choice.h
@@ -0,0 +1,16 @@
+/* SPDX-License-Identifier: MIT */
+/*
+ * Copyright © 2019 Intel Corporation
+ */
+
+#ifndef __STRING_CHOICE_H__
+#define __STRING_CHOICE_H__
+
+#include <linux/types.h>
+
+const char *yesno(bool v);
+const char *onoff(bool v);
+const char *enableddisabled(bool v);
+const char *plural(long v);
+
+#endif /* __STRING_CHOICE_H__ */
diff --git a/lib/Makefile b/lib/Makefile
index c5892807e06f..30247fbf1afe 100644
--- a/lib/Makefile
+++ b/lib/Makefile
@@ -46,7 +46,7 @@ obj-y += bcd.o sort.o parser.o debug_locks.o random32.o \
 	 bsearch.o find_bit.o llist.o memweight.o kfifo.o \
 	 percpu-refcount.o rhashtable.o \
 	 once.o refcount.o usercopy.o errseq.o bucket_locks.o \
-	 generic-radix-tree.o
+	 generic-radix-tree.o string-choice.o
 obj-$(CONFIG_STRING_SELFTEST) += test_string.o
 obj-y += string_helpers.o
 obj-$(CONFIG_TEST_STRING_HELPERS) += test-string_helpers.o
diff --git a/lib/string-choice.c b/lib/string-choice.c
new file mode 100644
index 000000000000..d20680a6603e
--- /dev/null
+++ b/lib/string-choice.c
@@ -0,0 +1,31 @@
+// SPDX-License-Identifier: MIT
+/*
+ * Copyright © 2019 Intel Corporation
+ */
+
+#include <linux/export.h>
+#include <linux/string-choice.h>
+
+const char *yesno(bool v)
+{
+	return v ? "yes" : "no";
+}
+EXPORT_SYMBOL(yesno);
+
+const char *onoff(bool v)
+{
+	return v ? "on" : "off";
+}
+EXPORT_SYMBOL(onoff);
+
+const char *enableddisabled(bool v)
+{
+	return v ? "enabled" : "disabled";
+}
+EXPORT_SYMBOL(enableddisabled);
+
+const char *plural(long v)
+{
+	return v == 1 ? "" : "s";
+}
+EXPORT_SYMBOL(plural);
-- 
2.20.1

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

[Rasmus Villemoes]

On 30/09/2019 16.18, Jani Nikula wrote:
> The kernel has plenty of ternary operators to choose between constant
> strings, such as condition ? "yes" : "no", as well as value == 1 ? "" :
> "s":
> 
> 
> ---
> 
> v2: add string-choice.[ch] to not clutter kernel.h and to actually save
> space on string constants.
> 
> +EXPORT_SYMBOL(enableddisabled);
> +
> +const char *plural(long v)
> +{
> +	return v == 1 ? "" : "s";
> +}
> +EXPORT_SYMBOL(plural);
> 


Say what? I'll bet you a beer that this is a net loss: You're adding
hundreds of bytes of export symbol overhead, plus forcing gcc to emit
actual calls at the call sites, with all the register saving/restoring
that implies.

Please just do this as static inlines. As I said, the linker is
perfectly capable of merging string literals across translation units
(but of course not between vmlinux and modules), so any built-in code
that uses those helpers (or open-codes them, doesn't matter) will
automatically share those literals.

Rasmus

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

[Jani Nikula]

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.

While the main goal here is to abstract recurring patterns, and slightly
clean up the code base by not open coding the ternary operators, there
are also some space savings to be had via better string constant
pooling.

Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Cc: intel-gfx@lists.freedesktop.org
Cc: Vishal Kulkarni <vishal@chelsio.com>
Cc: netdev@vger.kernel.org
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: linux-usb@vger.kernel.org
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: linux-kernel@vger.kernel.org
Cc: Julia Lawall <julia.lawall@lip6.fr>
Cc: Rasmus Villemoes <linux@rasmusvillemoes.dk>
Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> # v1
Signed-off-by: Jani Nikula <jani.nikula@intel.com>

---

v2: add string-choice.[ch] to not clutter kernel.h and to actually save
space on string constants.

v3: back to static inlines based on Rasmus' feedback

Example of further cleanup possibilities are at [1], to be done
incrementally afterwards.

[1] http://lore.kernel.org/r/20190903133731.2094-2-jani.nikula@intel.com
---
 drivers/gpu/drm/i915/i915_utils.h             | 16 +---------
 .../ethernet/chelsio/cxgb4/cxgb4_debugfs.c    | 12 +------
 drivers/usb/core/config.c                     |  6 +---
 drivers/usb/core/generic.c                    |  6 +---
 include/linux/string-choice.h                 | 31 +++++++++++++++++++
 5 files changed, 35 insertions(+), 36 deletions(-)
 create mode 100644 include/linux/string-choice.h

diff --git a/drivers/gpu/drm/i915/i915_utils.h b/drivers/gpu/drm/i915/i915_utils.h
index 562f756da421..794f02a90efe 100644
--- a/drivers/gpu/drm/i915/i915_utils.h
+++ b/drivers/gpu/drm/i915/i915_utils.h
@@ -28,6 +28,7 @@
 #include <linux/list.h>
 #include <linux/overflow.h>
 #include <linux/sched.h>
+#include <linux/string-choice.h>
 #include <linux/types.h>
 #include <linux/workqueue.h>
 
@@ -395,21 +396,6 @@ wait_remaining_ms_from_jiffies(unsigned long timestamp_jiffies, int to_wait_ms)
 #define MBps(x) KBps(1000 * (x))
 #define GBps(x) ((u64)1000 * MBps((x)))
 
-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 void add_taint_for_CI(unsigned int taint)
 {
 	/*
diff --git a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_debugfs.c b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_debugfs.c
index ae6a47dd7dc9..d9123dae1d00 100644
--- a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_debugfs.c
+++ b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_debugfs.c
@@ -35,6 +35,7 @@
 #include <linux/seq_file.h>
 #include <linux/debugfs.h>
 #include <linux/string_helpers.h>
+#include <linux/string-choice.h>
 #include <linux/sort.h>
 #include <linux/ctype.h>
 
@@ -2023,17 +2024,6 @@ static const struct file_operations rss_debugfs_fops = {
 /* RSS Configuration.
  */
 
-/* Small utility function to return the strings "yes" or "no" if the supplied
- * argument is non-zero.
- */
-static const char *yesno(int x)
-{
-	static const char *yes = "yes";
-	static const char *no = "no";
-
-	return x ? yes : no;
-}
-
 static int rss_config_show(struct seq_file *seq, void *v)
 {
 	struct adapter *adapter = seq->private;
diff --git a/drivers/usb/core/config.c b/drivers/usb/core/config.c
index 151a74a54386..52cee9067eb4 100644
--- a/drivers/usb/core/config.c
+++ b/drivers/usb/core/config.c
@@ -10,6 +10,7 @@
 #include <linux/module.h>
 #include <linux/slab.h>
 #include <linux/device.h>
+#include <linux/string-choice.h>
 #include <asm/byteorder.h>
 #include "usb.h"
 
@@ -19,11 +20,6 @@
 #define USB_MAXCONFIG			8	/* Arbitrary limit */
 
 
-static inline const char *plural(int n)
-{
-	return (n == 1 ? "" : "s");
-}
-
 static int find_next_descriptor(unsigned char *buffer, int size,
     int dt1, int dt2, int *num_skipped)
 {
diff --git a/drivers/usb/core/generic.c b/drivers/usb/core/generic.c
index 38f8b3e31762..a784a09794d6 100644
--- a/drivers/usb/core/generic.c
+++ b/drivers/usb/core/generic.c
@@ -21,14 +21,10 @@
 
 #include <linux/usb.h>
 #include <linux/usb/hcd.h>
+#include <linux/string-choice.h>
 #include <uapi/linux/usb/audio.h>
 #include "usb.h"
 
-static inline const char *plural(int n)
-{
-	return (n == 1 ? "" : "s");
-}
-
 static int is_rndis(struct usb_interface_descriptor *desc)
 {
 	return desc->bInterfaceClass == USB_CLASS_COMM
diff --git a/include/linux/string-choice.h b/include/linux/string-choice.h
new file mode 100644
index 000000000000..320b598bd8f0
--- /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__ */
-- 
2.20.1

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

[Greg Kroah-Hartman]

On Tue, Oct 01, 2019 at 11:07:39AM +0300, Jani Nikula 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.
> 
> While the main goal here is to abstract recurring patterns, and slightly
> clean up the code base by not open coding the ternary operators, there
> are also some space savings to be had via better string constant
> pooling.
> 
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Cc: intel-gfx@lists.freedesktop.org
> Cc: Vishal Kulkarni <vishal@chelsio.com>
> Cc: netdev@vger.kernel.org
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: linux-usb@vger.kernel.org
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: linux-kernel@vger.kernel.org
> Cc: Julia Lawall <julia.lawall@lip6.fr>
> Cc: Rasmus Villemoes <linux@rasmusvillemoes.dk>
> Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> # v1

As this is a totally different version, please drop my reviewed-by as
that's really not true here :(

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

[Jani Nikula]

On Tue, 01 Oct 2019, Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote:
> On Tue, Oct 01, 2019 at 11:07:39AM +0300, Jani Nikula 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.
>> 
>> While the main goal here is to abstract recurring patterns, and slightly
>> clean up the code base by not open coding the ternary operators, there
>> are also some space savings to be had via better string constant
>> pooling.
>> 
>> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
>> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
>> Cc: intel-gfx@lists.freedesktop.org
>> Cc: Vishal Kulkarni <vishal@chelsio.com>
>> Cc: netdev@vger.kernel.org
>> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>> Cc: linux-usb@vger.kernel.org
>> Cc: Andrew Morton <akpm@linux-foundation.org>
>> Cc: linux-kernel@vger.kernel.org
>> Cc: Julia Lawall <julia.lawall@lip6.fr>
>> Cc: Rasmus Villemoes <linux@rasmusvillemoes.dk>
>> Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> # v1
>
> As this is a totally different version, please drop my reviewed-by as
> that's really not true here :(

I did indicate it was for v1. Indeed v2 was different, but care to
elaborate what's wrong with v3?

BR,
Jani.


-- 
Jani Nikula, Intel Open Source Graphics Center

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

[Greg Kroah-Hartman]

On Tue, Oct 01, 2019 at 12:42:34PM +0300, Jani Nikula wrote:
> On Tue, 01 Oct 2019, Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote:
> > On Tue, Oct 01, 2019 at 11:07:39AM +0300, Jani Nikula 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.
> >> 
> >> While the main goal here is to abstract recurring patterns, and slightly
> >> clean up the code base by not open coding the ternary operators, there
> >> are also some space savings to be had via better string constant
> >> pooling.
> >> 
> >> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> >> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> >> Cc: intel-gfx@lists.freedesktop.org
> >> Cc: Vishal Kulkarni <vishal@chelsio.com>
> >> Cc: netdev@vger.kernel.org
> >> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> >> Cc: linux-usb@vger.kernel.org
> >> Cc: Andrew Morton <akpm@linux-foundation.org>
> >> Cc: linux-kernel@vger.kernel.org
> >> Cc: Julia Lawall <julia.lawall@lip6.fr>
> >> Cc: Rasmus Villemoes <linux@rasmusvillemoes.dk>
> >> Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> # v1
> >
> > As this is a totally different version, please drop my reviewed-by as
> > that's really not true here :(
> 
> I did indicate it was for v1. Indeed v2 was different, but care to
> elaborate what's wrong with v3?

No idea, but I haven't reviewed it yet, so to put my tag on there isn't
the nicest...

greg k-h

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

[Jani Nikula]

On Tue, 01 Oct 2019, Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote:
> On Tue, Oct 01, 2019 at 12:42:34PM +0300, Jani Nikula wrote:
>> On Tue, 01 Oct 2019, Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote:
>> > On Tue, Oct 01, 2019 at 11:07:39AM +0300, Jani Nikula 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.
>> >> 
>> >> While the main goal here is to abstract recurring patterns, and slightly
>> >> clean up the code base by not open coding the ternary operators, there
>> >> are also some space savings to be had via better string constant
>> >> pooling.
>> >> 
>> >> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
>> >> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
>> >> Cc: intel-gfx@lists.freedesktop.org
>> >> Cc: Vishal Kulkarni <vishal@chelsio.com>
>> >> Cc: netdev@vger.kernel.org
>> >> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>> >> Cc: linux-usb@vger.kernel.org
>> >> Cc: Andrew Morton <akpm@linux-foundation.org>
>> >> Cc: linux-kernel@vger.kernel.org
>> >> Cc: Julia Lawall <julia.lawall@lip6.fr>
>> >> Cc: Rasmus Villemoes <linux@rasmusvillemoes.dk>
>> >> Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> # v1
>> >
>> > As this is a totally different version, please drop my reviewed-by as
>> > that's really not true here :(
>> 
>> I did indicate it was for v1. Indeed v2 was different, but care to
>> elaborate what's wrong with v3?
>
> No idea, but I haven't reviewed it yet, so to put my tag on there isn't
> the nicest...

Apologies, no harm intended.

At times, I've seen the "# vN" notation used, I suppose both to indicate
that the *ideas* presented in the earlier version warranted Reviewed-by
from so-and-so, though this particular version still needs detailed
review, and that the approval of the reviewer of the earlier version
should be sought out before merging a subsequent version.

BR,
Jani.


-- 
Jani Nikula, Intel Open Source Graphics Center

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

[Jani Nikula]

On Tue, 01 Oct 2019, Jani Nikula <jani.nikula@intel.com> wrote:
> While the main goal here is to abstract recurring patterns, and slightly
> clean up the code base by not open coding the ternary operators, there
> are also some space savings to be had via better string constant
> pooling.

Make that

"""
While the main goal here is to abstract recurring patterns, and slightly
clean up the code base by not open coding the ternary operators, using
functions to access the strings also makes it easier to seek different
implementation options for potential space savings on string constants
in the future.
"""

to be more explicit that this change does not directly translate to any
space savings.

Rasmus, okay with that?


BR,
Jani.


-- 
Jani Nikula, Intel Open Source Graphics Center

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

[Rasmus Villemoes]

On 02/10/2019 12.11, Jani Nikula wrote:
> On Tue, 01 Oct 2019, Jani Nikula <jani.nikula@intel.com> wrote:
>> While the main goal here is to abstract recurring patterns, and slightly
>> clean up the code base by not open coding the ternary operators, there
>> are also some space savings to be had via better string constant
>> pooling.
> 
> Make that
> 
> """
> While the main goal here is to abstract recurring patterns, and slightly
> clean up the code base by not open coding the ternary operators, using
> functions to access the strings also makes it easier to seek different
> implementation options for potential space savings on string constants
> in the future.
> """
> 
> to be more explicit that this change does not directly translate to any
> space savings.
> 
> Rasmus, okay with that?

It's rather fluffy, but it doesn't make unfounded claims about space
savings, so in that regard I'm fine with it.

[It's probably just my lack of imagination, but I still fail to see how
one could ever achieve better than the linker creating just 1
vmlinux-wide instance of "enabled", which I believe happens regardless
of whether one uses these helpers or not.]

Rasmus

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

[Greg Kroah-Hartman]

On Tue, Oct 01, 2019 at 11:07:39AM +0300, Jani Nikula 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.
> 
> While the main goal here is to abstract recurring patterns, and slightly
> clean up the code base by not open coding the ternary operators, there
> are also some space savings to be had via better string constant
> pooling.
> 
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Cc: intel-gfx@lists.freedesktop.org
> Cc: Vishal Kulkarni <vishal@chelsio.com>
> Cc: netdev@vger.kernel.org
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: linux-usb@vger.kernel.org
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: linux-kernel@vger.kernel.org
> Cc: Julia Lawall <julia.lawall@lip6.fr>
> Cc: Rasmus Villemoes <linux@rasmusvillemoes.dk>
> Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> # v1
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>

Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

For this version at least :)