[PATCH 0/2] lib: add sputchar() helper

[PATCH 0/2] lib: add sputchar() helper

[Yury Norov]

This series adds sputchar() helper and coccinelle script that I used to
find candidates for cleanup.

Yury Norov (2): 
  lib/vsprintf: add sputchar()
  coccinelle script

 include/linux/kernel.h                 |   8 +
 lib/string_helpers.c                   |  56 ++-----
 lib/vsprintf.c                         | 222 ++++++++-----------------
 scripts/coccinelle/misc/sputchar.cocci | 190 +++++++++++++++++++++
 4 files changed, 280 insertions(+), 196 deletions(-)
 create mode 100644 scripts/coccinelle/misc/sputchar.cocci

-- 
2.30.2

[PATCH v2 1/2] lib: add sputchar() helper

[Yury Norov]

There are 47 occurrences of the code snippet like this:
	if (buf < end)
	        *buf = ' ';
	++buf;

This patch adds a helper function sputchar() to replace opencoding.
It adds a lot to readability, and also saves 43 bytes of text on x86.

v2: cleanup cases discovered with coccinelle script.

Signed-off-by: Yury Norov <yury.norov@gmail.com>
---
 include/linux/kernel.h |   8 ++
 lib/string_helpers.c   |  56 +++--------
 lib/vsprintf.c         | 222 +++++++++++++----------------------------
 3 files changed, 90 insertions(+), 196 deletions(-)

diff --git a/include/linux/kernel.h b/include/linux/kernel.h
index 1b2f0a7e00d6..01941ed2f5aa 100644
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h
@@ -283,6 +283,14 @@ extern char *bin2hex(char *dst, const void *src, size_t count);
 
 bool mac_pton(const char *s, u8 *mac);
 
+static inline char *sputchar(char *buf, const char *end, char c)
+{
+	if (buf < end)
+		*buf = c;
+
+	return buf + 1;
+}
+
 /*
  * General tracing related utility functions - trace_printk(),
  * tracing_on/tracing_off and tracing_start()/tracing_stop
diff --git a/lib/string_helpers.c b/lib/string_helpers.c
index 3806a52ce697..f4e0c719650d 100644
--- a/lib/string_helpers.c
+++ b/lib/string_helpers.c
@@ -335,12 +335,8 @@ static bool escape_space(unsigned char c, char **dst, char *end)
 		return false;
 	}
 
-	if (out < end)
-		*out = '\\';
-	++out;
-	if (out < end)
-		*out = to;
-	++out;
+	out = sputchar(out, end, '\\');
+	out = sputchar(out, end, to);
 
 	*dst = out;
 	return true;
@@ -368,12 +364,8 @@ static bool escape_special(unsigned char c, char **dst, char *end)
 		return false;
 	}
 
-	if (out < end)
-		*out = '\\';
-	++out;
-	if (out < end)
-		*out = to;
-	++out;
+	out = sputchar(out, end, '\\');
+	out = sputchar(out, end, to);
 
 	*dst = out;
 	return true;
@@ -386,12 +378,8 @@ static bool escape_null(unsigned char c, char **dst, char *end)
 	if (c)
 		return false;
 
-	if (out < end)
-		*out = '\\';
-	++out;
-	if (out < end)
-		*out = '0';
-	++out;
+	out = sputchar(out, end, '\\');
+	out = sputchar(out, end, '0');
 
 	*dst = out;
 	return true;
@@ -401,18 +389,10 @@ static bool escape_octal(unsigned char c, char **dst, char *end)
 {
 	char *out = *dst;
 
-	if (out < end)
-		*out = '\\';
-	++out;
-	if (out < end)
-		*out = ((c >> 6) & 0x07) + '0';
-	++out;
-	if (out < end)
-		*out = ((c >> 3) & 0x07) + '0';
-	++out;
-	if (out < end)
-		*out = ((c >> 0) & 0x07) + '0';
-	++out;
+	out = sputchar(out, end, '\\');
+	out = sputchar(out, end, ((c >> 6) & 0x07) + '0');
+	out = sputchar(out, end, ((c >> 3) & 0x07) + '0');
+	out = sputchar(out, end, ((c >> 0) & 0x07) + '0');
 
 	*dst = out;
 	return true;
@@ -422,18 +402,10 @@ static bool escape_hex(unsigned char c, char **dst, char *end)
 {
 	char *out = *dst;
 
-	if (out < end)
-		*out = '\\';
-	++out;
-	if (out < end)
-		*out = 'x';
-	++out;
-	if (out < end)
-		*out = hex_asc_hi(c);
-	++out;
-	if (out < end)
-		*out = hex_asc_lo(c);
-	++out;
+	out = sputchar(out, end, '\\');
+	out = sputchar(out, end, 'x');
+	out = sputchar(out, end, hex_asc_hi(c));
+	out = sputchar(out, end, hex_asc_lo(c));
 
 	*dst = out;
 	return true;
diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 919b70d00855..9d4c9fd81845 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -509,59 +509,38 @@ char *number(char *buf, char *end, unsigned long long num,
 	/* leading space padding */
 	field_width -= precision;
 	if (!(spec.flags & (ZEROPAD | LEFT))) {
-		while (--field_width >= 0) {
-			if (buf < end)
-				*buf = ' ';
-			++buf;
-		}
+		while (--field_width >= 0)
+			buf = sputchar(buf, end, ' ');
 	}
 	/* sign */
-	if (sign) {
-		if (buf < end)
-			*buf = sign;
-		++buf;
-	}
+	if (sign)
+		buf = sputchar(buf, end, sign);
+
 	/* "0x" / "0" prefix */
 	if (need_pfx) {
-		if (spec.base == 16 || !is_zero) {
-			if (buf < end)
-				*buf = '0';
-			++buf;
-		}
-		if (spec.base == 16) {
-			if (buf < end)
-				*buf = ('X' | locase);
-			++buf;
-		}
+		if (spec.base == 16 || !is_zero)
+			buf = sputchar(buf, end, '0');
+		if (spec.base == 16)
+			buf = sputchar(buf, end, 'X' | locase);
 	}
 	/* zero or space padding */
 	if (!(spec.flags & LEFT)) {
 		char c = ' ' + (spec.flags & ZEROPAD);
 
-		while (--field_width >= 0) {
-			if (buf < end)
-				*buf = c;
-			++buf;
-		}
+		while (--field_width >= 0)
+			buf = sputchar(buf, end, c);
 	}
 	/* hmm even more zero padding? */
-	while (i <= --precision) {
-		if (buf < end)
-			*buf = '0';
-		++buf;
-	}
+	while (i <= --precision)
+		buf = sputchar(buf, end, '0');
+
 	/* actual digits of result */
-	while (--i >= 0) {
-		if (buf < end)
-			*buf = tmp[i];
-		++buf;
-	}
+	while (--i >= 0)
+		buf = sputchar(buf, end, tmp[i]);
+
 	/* trailing space padding */
-	while (--field_width >= 0) {
-		if (buf < end)
-			*buf = ' ';
-		++buf;
-	}
+	while (--field_width >= 0)
+		buf = sputchar(buf, end, ' ');
 
 	return buf;
 }
@@ -619,11 +598,9 @@ char *widen_string(char *buf, int n, char *end, struct printf_spec spec)
 		move_right(buf - n, end, n, spaces);
 		return buf + spaces;
 	}
-	while (spaces--) {
-		if (buf < end)
-			*buf = ' ';
-		++buf;
-	}
+	while (spaces--)
+		buf = sputchar(buf, end, ' ');
+
 	return buf;
 }
 
@@ -638,9 +615,7 @@ static char *string_nocheck(char *buf, char *end, const char *s,
 		char c = *s++;
 		if (!c)
 			break;
-		if (buf < end)
-			*buf = c;
-		++buf;
+		buf = sputchar(buf, end, c);
 		++len;
 	}
 	return widen_string(buf, len, end, spec);
@@ -931,7 +906,7 @@ char *dentry_name(char *buf, char *end, const struct dentry *d, struct printf_sp
 		}
 	}
 	s = array[--i];
-	for (n = 0; n != spec.precision; n++, buf++) {
+	for (n = 0; n != spec.precision; n++) {
 		char c = *s++;
 		if (!c) {
 			if (!i)
@@ -939,8 +914,7 @@ char *dentry_name(char *buf, char *end, const struct dentry *d, struct printf_sp
 			c = '/';
 			s = array[--i];
 		}
-		if (buf < end)
-			*buf = c;
+		buf = sputchar(buf, end, c);
 	}
 	rcu_read_unlock();
 	return widen_string(buf, n, end, spec);
@@ -968,11 +942,9 @@ char *bdev_name(char *buf, char *end, struct block_device *bdev,
 	hd = bdev->bd_disk;
 	buf = string(buf, end, hd->disk_name, spec);
 	if (bdev->bd_partno) {
-		if (isdigit(hd->disk_name[strlen(hd->disk_name)-1])) {
-			if (buf < end)
-				*buf = 'p';
-			buf++;
-		}
+		if (isdigit(hd->disk_name[strlen(hd->disk_name)-1]))
+			buf = sputchar(buf, end, 'p');
+
 		buf = number(buf, end, bdev->bd_partno, spec);
 	}
 	return buf;
@@ -1175,18 +1147,10 @@ char *hex_string(char *buf, char *end, u8 *addr, struct printf_spec spec,
 		len = min_t(int, spec.field_width, 64);
 
 	for (i = 0; i < len; ++i) {
-		if (buf < end)
-			*buf = hex_asc_hi(addr[i]);
-		++buf;
-		if (buf < end)
-			*buf = hex_asc_lo(addr[i]);
-		++buf;
-
-		if (separator && i != len - 1) {
-			if (buf < end)
-				*buf = separator;
-			++buf;
-		}
+		buf = sputchar(buf, end, hex_asc_hi(addr[i]));
+		buf = sputchar(buf, end, hex_asc_lo(addr[i]));
+		if (separator && i != len - 1)
+			buf = sputchar(buf, end, separator);
 	}
 
 	return buf;
@@ -1221,11 +1185,9 @@ char *bitmap_string(char *buf, char *end, unsigned long *bitmap,
 		bit = i % BITS_PER_LONG;
 		val = (bitmap[word] >> bit) & chunkmask;
 
-		if (!first) {
-			if (buf < end)
-				*buf = ',';
-			buf++;
-		}
+		if (!first)
+			buf = sputchar(buf, end, ',');
+
 		first = false;
 
 		spec.field_width = DIV_ROUND_UP(chunksz, 4);
@@ -1248,20 +1210,17 @@ char *bitmap_list_string(char *buf, char *end, unsigned long *bitmap,
 		return buf;
 
 	for_each_set_bitrange(rbot, rtop, bitmap, nr_bits) {
-		if (!first) {
-			if (buf < end)
-				*buf = ',';
-			buf++;
-		}
+		if (!first)
+			buf = sputchar(buf, end, ',');
+
 		first = false;
 
 		buf = number(buf, end, rbot, default_dec_spec);
 		if (rtop == rbot + 1)
 			continue;
 
-		if (buf < end)
-			*buf = '-';
-		buf = number(buf + 1, end, rtop - 1, default_dec_spec);
+		buf = sputchar(buf, end, '-');
+		buf = number(buf, end, rtop - 1, default_dec_spec);
 	}
 	return buf;
 }
@@ -1822,14 +1781,9 @@ char *date_str(char *buf, char *end, const struct rtc_time *tm, bool r)
 	int mon = tm->tm_mon + (r ? 0 : 1);
 
 	buf = number(buf, end, year, default_dec04_spec);
-	if (buf < end)
-		*buf = '-';
-	buf++;
-
+	buf = sputchar(buf, end, '-');
 	buf = number(buf, end, mon, default_dec02_spec);
-	if (buf < end)
-		*buf = '-';
-	buf++;
+	buf = sputchar(buf, end, '-');
 
 	return number(buf, end, tm->tm_mday, default_dec02_spec);
 }
@@ -1838,14 +1792,9 @@ static noinline_for_stack
 char *time_str(char *buf, char *end, const struct rtc_time *tm, bool r)
 {
 	buf = number(buf, end, tm->tm_hour, default_dec02_spec);
-	if (buf < end)
-		*buf = ':';
-	buf++;
-
+	buf = sputchar(buf, end, ':');
 	buf = number(buf, end, tm->tm_min, default_dec02_spec);
-	if (buf < end)
-		*buf = ':';
-	buf++;
+	buf = sputchar(buf, end, ':');
 
 	return number(buf, end, tm->tm_sec, default_dec02_spec);
 }
@@ -1889,11 +1838,8 @@ char *rtc_str(char *buf, char *end, const struct rtc_time *tm,
 
 	if (have_d)
 		buf = date_str(buf, end, tm, raw);
-	if (have_d && have_t) {
-		if (buf < end)
-			*buf = iso8601_separator ? 'T' : ' ';
-		buf++;
-	}
+	if (have_d && have_t)
+		buf = sputchar(buf, end, iso8601_separator ? 'T' : ' ');
 	if (have_t)
 		buf = time_str(buf, end, tm, raw);
 
@@ -1972,11 +1918,8 @@ char *format_flags(char *buf, char *end, unsigned long flags,
 		buf = string(buf, end, names->name, default_str_spec);
 
 		flags &= ~mask;
-		if (flags) {
-			if (buf < end)
-				*buf = '|';
-			buf++;
-		}
+		if (flags)
+			buf = sputchar(buf, end, '|');
 	}
 
 	if (flags)
@@ -2026,16 +1969,11 @@ char *format_page_flags(char *buf, char *end, unsigned long flags)
 			continue;
 
 		/* Format: Flag Name + '=' (equals sign) + Number + '|' (separator) */
-		if (append) {
-			if (buf < end)
-				*buf = '|';
-			buf++;
-		}
+		if (append)
+			buf = sputchar(buf, end, '|');
 
 		buf = string(buf, end, pff[i].name, default_str_spec);
-		if (buf < end)
-			*buf = '=';
-		buf++;
+		buf = sputchar(buf, end, '=');
 		buf = number(buf, end, (flags >> pff[i].shift) & pff[i].mask,
 			     *pff[i].spec);
 
@@ -2125,11 +2063,8 @@ char *device_node_string(char *buf, char *end, struct device_node *dn,
 
 	for (pass = false; strspn(fmt,"fnpPFcC"); fmt++, pass = true) {
 		int precision;
-		if (pass) {
-			if (buf < end)
-				*buf = ':';
-			buf++;
-		}
+		if (pass)
+			buf = sputchar(buf, end, ':');
 
 		switch (*fmt) {
 		case 'f':	/* full_name */
@@ -2773,25 +2708,14 @@ int vsnprintf(char *buf, size_t size, const char *fmt, va_list args)
 			break;
 
 		case FORMAT_TYPE_CHAR: {
-			char c;
-
 			if (!(spec.flags & LEFT)) {
-				while (--spec.field_width > 0) {
-					if (str < end)
-						*str = ' ';
-					++str;
-
-				}
-			}
-			c = (unsigned char) va_arg(args, int);
-			if (str < end)
-				*str = c;
-			++str;
-			while (--spec.field_width > 0) {
-				if (str < end)
-					*str = ' ';
-				++str;
+				while (--spec.field_width > 0)
+					str = sputchar(str, end, ' ');
 			}
+			str = sputchar(str, end, (unsigned char) va_arg(args, int));
+
+			while (--spec.field_width > 0)
+				str = sputchar(str, end, ' ');
 			break;
 		}
 
@@ -2807,9 +2731,7 @@ int vsnprintf(char *buf, size_t size, const char *fmt, va_list args)
 			break;
 
 		case FORMAT_TYPE_PERCENT_CHAR:
-			if (str < end)
-				*str = '%';
-			++str;
+			str = sputchar(str, end, '%');
 			break;
 
 		case FORMAT_TYPE_INVALID:
@@ -3251,21 +3173,15 @@ int bstr_printf(char *buf, size_t size, const char *fmt, const u32 *bin_buf)
 			char c;
 
 			if (!(spec.flags & LEFT)) {
-				while (--spec.field_width > 0) {
-					if (str < end)
-						*str = ' ';
-					++str;
-				}
+				while (--spec.field_width > 0)
+					str = sputchar(str, end, ' ');
 			}
+
 			c = (unsigned char) get_arg(char);
-			if (str < end)
-				*str = c;
-			++str;
-			while (--spec.field_width > 0) {
-				if (str < end)
-					*str = ' ';
-				++str;
-			}
+			str = sputchar(str, end, c);
+
+			while (--spec.field_width > 0)
+				str = sputchar(str, end, ' ');
 			break;
 		}
 
@@ -3312,9 +3228,7 @@ int bstr_printf(char *buf, size_t size, const char *fmt, const u32 *bin_buf)
 		}
 
 		case FORMAT_TYPE_PERCENT_CHAR:
-			if (str < end)
-				*str = '%';
-			++str;
+			str = sputchar(str, end, '%');
 			break;
 
 		case FORMAT_TYPE_INVALID:
-- 
2.30.2

[PATCH 2/2] coccinelle: add script for sputchar()

[Yury Norov]

This script find 47 candidates for sputchar() replacement, 
none of them is false-positive.

Suggested-by: Andy Shevchenko <andy.shevchenko@gmail.com>
Signed-off-by: Yury Norov <yury.norov@gmail.com>
---
One test case is commented-out because it causes spatch crash.
Coccinelle is installed from ubuntu deb package. 

yury:linux$ spatch --version
spatch version 1.0.8 compiled with OCaml version 4.11.1
Flags passed to the configure script: --prefix=/usr --sysconfdir=/etc --libdir=/usr/lib --enable-ocaml --enable-python --enable-opt
OCaml scripting support: yes
Python scripting support: yes
Syntax of regular expressions: PCRE

 scripts/coccinelle/misc/sputchar.cocci | 190 +++++++++++++++++++++++++
 1 file changed, 190 insertions(+)
 create mode 100644 scripts/coccinelle/misc/sputchar.cocci

diff --git a/scripts/coccinelle/misc/sputchar.cocci b/scripts/coccinelle/misc/sputchar.cocci
new file mode 100644
index 000000000000..23fb7546252f
--- /dev/null
+++ b/scripts/coccinelle/misc/sputchar.cocci
@@ -0,0 +1,190 @@
+// SPDX-License-Identifier: GPL-2.0-only
+///
+/// Check for opencoded sputchar() implementation.
+///
+// Confidence: High
+// Copyright: (C) 2021 Yury Norov
+// Options: --no-includes --include-headers
+//
+// Keywords: sputchar
+//
+
+virtual patch
+virtual org
+virtual report
+virtual context
+
+@rpostfix depends on !patch@
+identifier func;
+expression buf, end, c;
+position p;
+@@
+
+func(...)
+{
+	<...
+*	if ((buf) < (end)) {
+*		*(buf) = (c);
+*	}
+*	(buf)++;@p
+	...>
+}
+
+@rprefix depends on !patch@
+identifier func;
+expression buf, end, c;
+position p;
+@@
+
+func(...)
+{
+	<...
+*	if ((buf) < (end))
+*		*(buf) = (c);
+*	++(buf);@p
+	...>
+}
+
+@rinc1 depends on !patch@
+identifier func;
+expression buf, end, c;
+position p;
+@@
+
+func(...)
+{
+	<...
+*	if ((buf) < (end)) {
+*		*(buf) = (c);
+*	}
+*	(buf) += 1;@p
+	...>
+}
+
+@rinc2 depends on !patch@
+identifier func;
+expression buf, end, c;
+position p;
+@@
+
+func(...)
+{
+	<...
+*	if ((buf) < (end)) {
+*		*(buf) = (c);
+*	}
+*	(buf) = (buf) + 1;@p
+	...>
+}
+
+@ppostfix depends on patch@
+identifier func;
+expression buf, end, c;
+position p;
+@@
+
+func(...)
+{
+	<...
+-	if ((buf) < (end)) {
+-		*(buf) = (c);
+-	}
+-	(buf)++;@p
++	buf = sputchar(buf, end, c);
+	...>
+}
+
+// @pprefix depends on patch@
+// identifier func;
+// expression buf, end, c;
+// position p;
+// @@
+//
+// func(...)
+// {
+// 	<...
+// -	if ((buf) < (end)) {
+// -		*(buf) = (c);
+// -	}
+// -	++(buf);
+// +	buf = sputchar(buf, end, c);
+// 	...>
+// }
+
+@pinc1 depends on patch@
+identifier func;
+expression buf, end, c;
+position p;
+@@
+
+func(...)
+{
+	<...
+-	if ((buf) < (end)) {
+-		*(buf) = (c);
+-	}
+-	(buf) += 1;
++	buf = sputchar(buf, end, c);
+	...>
+}
+
+@pinc2 depends on patch@
+identifier func;
+expression buf, end, c;
+position p;
+@@
+
+func(...)
+{
+	<...
+-	if ((buf) < (end)) {
+-		*(buf) = (c);
+-	}
+-	(buf) = (buf) + 1;
++	buf = sputchar(buf, end, c);
+	...>
+}
+
+@script:python depends on report@
+p << rpostfix.p;
+@@
+
+for p0 in p:
+	coccilib.report.print_report(p0, "WARNING opportunity for sputchar()")
+
+@script:python depends on org@
+p << rpostfix.p;
+@@
+
+for p0 in p:
+	coccilib.report.print_report(p0, "WARNING opportunity for sputchar()")
+
+
+@script:python depends on report@
+p << rprefix.p;
+@@
+
+for p0 in p:
+	coccilib.report.print_report(p0, "WARNING opportunity for sputchar()")
+
+@script:python depends on org@
+p << rprefix.p;
+@@
+
+for p0 in p:
+	coccilib.report.print_report(p0, "WARNING opportunity for sputchar()")
+
+@script:python depends on report@
+p << rinc1.p;
+@@
+
+for p0 in p:
+	coccilib.report.print_report(p0, "WARNING opportunity for sputchar()")
+
+@script:python depends on org@
+p << rinc1.p;
+@@
+
+for p0 in p:
+	coccilib.report.print_report(p0, "WARNING opportunity for sputchar()")
+
-- 
2.30.2

Re: [PATCH v2 1/2] lib: add sputchar() helper

[Joe Perches]

On Sat, 2021-09-04 at 16:10 -0700, Yury Norov wrote:
> There are 47 occurrences of the code snippet like this:
> 	if (buf < end)
> 	        *buf = ' ';
> 	++buf;
> 
> This patch adds a helper function sputchar() to replace opencoding.
> It adds a lot to readability, and also saves 43 bytes of text on x86.

I think this patch does little to improve readability.

Perhaps make it void and use something like

	sputchar(*buf++, end, <whateverchar>);

Though the sputchar name doesn't seems particularly intelligible.

> +static inline char *sputchar(char *buf, const char *end, char c)
> +{
> +	if (buf < end)
> +		*buf = c;
> +
> +	return buf + 1;
> +}
> +
>  /*
>   * General tracing related utility functions - trace_printk(),
>   * tracing_on/tracing_off and tracing_start()/tracing_stop
> diff --git a/lib/string_helpers.c b/lib/string_helpers.c
[]
> @@ -335,12 +335,8 @@ static bool escape_space(unsigned char c, char **dst, char *end)
>  		return false;
>  	}
> 
> -	if (out < end)
> -		*out = '\\';
> -	++out;
> -	if (out < end)
> -		*out = to;
> -	++out;
> +	out = sputchar(out, end, '\\');
> +	out = sputchar(out, end, to);

etc...

Re: [PATCH v2 1/2] lib: add sputchar() helper

[Yury Norov]

On Sat, Sep 04, 2021 at 06:36:41PM -0700, Joe Perches wrote:
> On Sat, 2021-09-04 at 16:10 -0700, Yury Norov wrote:
> > There are 47 occurrences of the code snippet like this:
> > 	if (buf < end)
> > 	        *buf = ' ';
> > 	++buf;
> > 
> > This patch adds a helper function sputchar() to replace opencoding.
> > It adds a lot to readability, and also saves 43 bytes of text on x86.
> 
> I think this patch does little to improve readability.
> 
> Perhaps make it void and use something like
> 
> 	sputchar(*buf++, end, <whateverchar>);

It's better, thank you.
 
> Though the sputchar name doesn't seems particularly intelligible.

I'm OK with any better name.

Thanks,
Yury

> > +static inline char *sputchar(char *buf, const char *end, char c)
> > +{
> > +	if (buf < end)
> > +		*buf = c;
> > +
> > +	return buf + 1;
> > +}
> > +
> >  /*
> >   * General tracing related utility functions - trace_printk(),
> >   * tracing_on/tracing_off and tracing_start()/tracing_stop
> > diff --git a/lib/string_helpers.c b/lib/string_helpers.c
> []
> > @@ -335,12 +335,8 @@ static bool escape_space(unsigned char c, char **dst, char *end)
> >  		return false;
> >  	}
> > 
> > -	if (out < end)
> > -		*out = '\\';
> > -	++out;
> > -	if (out < end)
> > -		*out = to;
> > -	++out;
> > +	out = sputchar(out, end, '\\');
> > +	out = sputchar(out, end, to);
> 
> etc...
> 

Re: [PATCH v2 1/2] lib: add sputchar() helper

[Rasmus Villemoes]

On 05/09/2021 01.10, Yury Norov wrote:
> There are 47 occurrences of the code snippet like this:
> 	if (buf < end)
> 	        *buf = ' ';
> 	++buf;
> 
> This patch adds a helper function sputchar() to replace opencoding.
> It adds a lot to readability, and also saves 43 bytes of text on x86.
> 
> v2: cleanup cases discovered with coccinelle script.
> 
> Signed-off-by: Yury Norov <yury.norov@gmail.com>
> ---
>  include/linux/kernel.h |   8 ++

Sorry, but 47 cases, mostly in one .c file, is not enough justification
for adding yet another piece of random code to
the-dumping-ground-which-is-kernel.h, especially since this helper is
very specific to the needs of the vsnprintf() implementation, so
extremely unlikely to find other users.

I'm also not a fan of the sputchar name - it's unreadable at first
glance, and when you figure out it's "a _s_tring version of putchar",
that doesn't help, because its semantics are nothing like the stdio putchar.

Rasmus

Re: [PATCH v2 1/2] lib: add sputchar() helper

[Petr Mladek]

On Mon 2021-09-06 13:51:59, Rasmus Villemoes wrote:
> On 05/09/2021 01.10, Yury Norov wrote:
> > There are 47 occurrences of the code snippet like this:
> > 	if (buf < end)
> > 	        *buf = ' ';
> > 	++buf;
> > 
> > This patch adds a helper function sputchar() to replace opencoding.
> > It adds a lot to readability, and also saves 43 bytes of text on x86.
> > 
> > v2: cleanup cases discovered with coccinelle script.
> > 
> > Signed-off-by: Yury Norov <yury.norov@gmail.com>
> > ---
> >  include/linux/kernel.h |   8 ++
> 
> Sorry, but 47 cases, mostly in one .c file, is not enough justification
> for adding yet another piece of random code to
> the-dumping-ground-which-is-kernel.h, especially since this helper is
> very specific to the needs of the vsnprintf() implementation, so
> extremely unlikely to find other users.

What about putting it into include/linux/string_helpers.h ?

Or create include/linux/vsprintf.h ?

> I'm also not a fan of the sputchar name - it's unreadable at first
> glance, and when you figure out it's "a _s_tring version of putchar",
> that doesn't help, because its semantics are nothing like the stdio putchar.

I do not like the name either.

What about vsprintf_putc(buf, end, c) or vsp_putc(buf, end, c)?

Well, the ugly thing are the three parameters. Which brings an idea of

	struct vsprintf_buffer {       // or struct vsp_buf
		char *buf,
		char *end,
	}

and using vprintf_putc(vsp_buf, c) or vsp_putc(vsp_buf, c).

The change would look like:

From 32119545392f560be42c7042721811a3177fb1dc Mon Sep 17 00:00:00 2001
From: Petr Mladek <pmladek@suse.com>
Date: Tue, 7 Sep 2021 11:31:22 +0200
Subject: [PATCH] vsprintf: Sample use of struct vsp_buf

This is just partial replacement of [buf,end] couple with
struct vsp_buf.

The intention is to see how the code would look like. It does
not compile.
---
 lib/vsprintf.c | 85 +++++++++++++++++++++-----------------------------
 1 file changed, 35 insertions(+), 50 deletions(-)

diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 3bcb7be03f93..963c9212141d 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -446,8 +446,9 @@ static_assert(sizeof(struct printf_spec) == 8);
 #define PRECISION_MAX ((1 << 15) - 1)
 
 static noinline_for_stack
-char *number(char *buf, char *end, unsigned long long num,
-	     struct printf_spec spec)
+strcut vsp_buf *number(struct vsp_buf *vsp_buf,
+		       unsigned long long num,
+		       struct printf_spec spec)
 {
 	/* put_dec requires 2-byte alignment of the buffer. */
 	char tmp[3 * sizeof(num)] __aligned(2);
@@ -506,68 +507,52 @@ char *number(char *buf, char *end, unsigned long long num,
 	/* printing 100 using %2d gives "100", not "00" */
 	if (i > precision)
 		precision = i;
+
 	/* leading space padding */
 	field_width -= precision;
 	if (!(spec.flags & (ZEROPAD | LEFT))) {
-		while (--field_width >= 0) {
-			if (buf < end)
-				*buf = ' ';
-			++buf;
-		}
+		while (--field_width >= 0)
+			vsp_putc(vsp_buf, ' ');
 	}
+
 	/* sign */
-	if (sign) {
-		if (buf < end)
-			*buf = sign;
-		++buf;
-	}
+	if (sign)
+		vsp_putc(vsp_buf, sign);
+
 	/* "0x" / "0" prefix */
 	if (need_pfx) {
-		if (spec.base == 16 || !is_zero) {
-			if (buf < end)
-				*buf = '0';
-			++buf;
-		}
+		if (spec.base == 16 || !is_zero)
+			vsp_putc(vps_buf, '0');
 		if (spec.base == 16) {
-			if (buf < end)
-				*buf = ('X' | locase);
-			++buf;
-		}
+			vsp_putc(vps_buf, 'X' | locase);
 	}
+
 	/* zero or space padding */
 	if (!(spec.flags & LEFT)) {
 		char c = ' ' + (spec.flags & ZEROPAD);
 
-		while (--field_width >= 0) {
-			if (buf < end)
-				*buf = c;
-			++buf;
-		}
+		while (--field_width >= 0)
+			vsp_putc(vps_buf, c);
 	}
+
 	/* hmm even more zero padding? */
-	while (i <= --precision) {
-		if (buf < end)
-			*buf = '0';
-		++buf;
-	}
+	while (i <= --precision)
+		vsp_putc(vps_buf, '0');
+
 	/* actual digits of result */
-	while (--i >= 0) {
-		if (buf < end)
-			*buf = tmp[i];
-		++buf;
-	}
+	while (--i >= 0)
+		vsp_putc(vps_buf, tmp[i]);
+
 	/* trailing space padding */
-	while (--field_width >= 0) {
-		if (buf < end)
-			*buf = ' ';
-		++buf;
-	}
+	while (--field_width >= 0)
+		vsp_putc(vps_buf, ' ');
 
 	return buf;
 }
 
 static noinline_for_stack
-char *special_hex_number(char *buf, char *end, unsigned long long num, int size)
+	struct vsp_buf* *special_hex_number(struct vsp_buf *vsp_buf,
+					    unsigned long long num, int size)
 {
 	struct printf_spec spec;
 
@@ -577,7 +562,7 @@ char *special_hex_number(char *buf, char *end, unsigned long long num, int size)
 	spec.base = 16;
 	spec.precision = -1;
 
-	return number(buf, end, num, spec);
+	return number(vsp_buf, num, spec);
 }
 
 static void move_right(char *buf, char *end, unsigned len, unsigned spaces)
@@ -646,14 +631,14 @@ static char *string_nocheck(char *buf, char *end, const char *s,
 	return widen_string(buf, len, end, spec);
 }
 
-static char *err_ptr(char *buf, char *end, void *ptr,
+static struct vsp_buf *err_ptr(struct vsp_buf *vsp_buf, void *ptr,
 		     struct printf_spec spec)
 {
 	int err = PTR_ERR(ptr);
 	const char *sym = errname(err);
 
 	if (sym)
-		return string_nocheck(buf, end, sym, spec);
+		return string_nocheck(vsp_buf, sym, spec);
 
 	/*
 	 * Somebody passed ERR_PTR(-1234) or some other non-existing
@@ -662,7 +647,7 @@ static char *err_ptr(char *buf, char *end, void *ptr,
 	 */
 	spec.flags |= SIGN;
 	spec.base = 10;
-	return number(buf, end, err, spec);
+	return number(vsp_buf, err, spec);
 }
 
 /* Be careful: error messages must fit into the given buffer. */
@@ -720,9 +705,9 @@ char *string(char *buf, char *end, const char *s,
 	return string_nocheck(buf, end, s, spec);
 }
 
-static char *pointer_string(char *buf, char *end,
-			    const void *ptr,
-			    struct printf_spec spec)
+static vsp_buf *pointer_string(struct vsp_buf *vsp_buf,
+			       const void *ptr,
+			       struct printf_spec spec)
 {
 	spec.base = 16;
 	spec.flags |= SMALL;
@@ -731,7 +716,7 @@ static char *pointer_string(char *buf, char *end,
 		spec.flags |= ZEROPAD;
 	}
 
-	return number(buf, end, (unsigned long int)ptr, spec);
+	return number(vsp_buf, (unsigned long int)ptr, spec);
 }
 
 /* Make pointers available for printing early in the boot sequence. */
-- 
2.26.2