[PATCH] lib/vsprintf: add __putchar()

[PATCH] lib/vsprintf: add __putchar()

[Yury Norov]

There are 26 occurrences of the code snippet like this in the file :
	if (buf < end)
	        *buf = separator;
	++buf;

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

Signed-off-by: Yury Norov <yury.norov@gmail.com>
---
 lib/vsprintf.c | 174 +++++++++++++++++--------------------------------
 1 file changed, 59 insertions(+), 115 deletions(-)

diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 134216c45980..58a985686c08 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -53,6 +53,14 @@
 #include <linux/string_helpers.h>
 #include "kstrtox.h"
 
+static inline char *__putchar(char *buf, const char *end, char c)
+{
+	if (buf < end)
+		*buf = c;
+
+	return buf + 1;
+}
+
 static unsigned long long simple_strntoull(const char *startp, size_t max_chars,
 					   char **endp, unsigned int base)
 {
@@ -509,59 +517,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 = __putchar(buf, end, ' ');
 	}
 	/* sign */
-	if (sign) {
-		if (buf < end)
-			*buf = sign;
-		++buf;
-	}
+	if (sign)
+		buf = __putchar(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 = __putchar(buf, end, '0');
+		if (spec.base == 16)
+			buf = __putchar(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 = __putchar(buf, end, c);
 	}
 	/* hmm even more zero padding? */
-	while (i <= --precision) {
-		if (buf < end)
-			*buf = '0';
-		++buf;
-	}
+	while (i <= --precision)
+		buf = __putchar(buf, end, '0');
+
 	/* actual digits of result */
-	while (--i >= 0) {
-		if (buf < end)
-			*buf = tmp[i];
-		++buf;
-	}
+	while (--i >= 0)
+		buf = __putchar(buf, end, tmp[i]);
+
 	/* trailing space padding */
-	while (--field_width >= 0) {
-		if (buf < end)
-			*buf = ' ';
-		++buf;
-	}
+	while (--field_width >= 0)
+		buf = __putchar(buf, end, ' ');
 
 	return buf;
 }
@@ -619,11 +606,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 = __putchar(buf, end, ' ');
+
 	return buf;
 }
 
@@ -638,9 +623,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 = __putchar(buf, end, c);
 		++len;
 	}
 	return widen_string(buf, len, end, spec);
@@ -968,11 +951,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 = __putchar(buf, end, 'p');
+
 		buf = number(buf, end, bdev->bd_partno, spec);
 	}
 	return buf;
@@ -1175,18 +1156,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 = __putchar(buf, end, hex_asc_hi(addr[i]));
+		buf = __putchar(buf, end, hex_asc_lo(addr[i]));
+		if (separator && i != len - 1)
+			buf = __putchar(buf, end, separator);
 	}
 
 	return buf;
@@ -1221,11 +1194,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 = __putchar(buf, end, ',');
+
 		first = false;
 
 		spec.field_width = DIV_ROUND_UP(chunksz, 4);
@@ -1248,20 +1219,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 = __putchar(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 = __putchar(buf, end, '-');
+		buf = number(buf, end, rtop - 1, default_dec_spec);
 	}
 	return buf;
 }
@@ -1822,14 +1790,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 = __putchar(buf, end, '-');
 	buf = number(buf, end, mon, default_dec02_spec);
-	if (buf < end)
-		*buf = '-';
-	buf++;
+	buf = __putchar(buf, end, '-');
 
 	return number(buf, end, tm->tm_mday, default_dec02_spec);
 }
@@ -1838,14 +1801,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 = __putchar(buf, end, ':');
 	buf = number(buf, end, tm->tm_min, default_dec02_spec);
-	if (buf < end)
-		*buf = ':';
-	buf++;
+	buf = __putchar(buf, end, ':');
 
 	return number(buf, end, tm->tm_sec, default_dec02_spec);
 }
@@ -1889,11 +1847,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 = __putchar(buf, end, iso8601_separator ? 'T' : ' ');
 	if (have_t)
 		buf = time_str(buf, end, tm, raw);
 
@@ -1972,11 +1927,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 = __putchar(buf, end, '|');
 	}
 
 	if (flags)
@@ -2026,16 +1978,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 = __putchar(buf, end, '|');
 
 		buf = string(buf, end, pff[i].name, default_str_spec);
-		if (buf < end)
-			*buf = '=';
-		buf++;
+		buf = __putchar(buf, end, '=');
 		buf = number(buf, end, (flags >> pff[i].shift) & pff[i].mask,
 			     *pff[i].spec);
 
@@ -2125,11 +2072,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 = __putchar(buf, end, ':');
 
 		switch (*fmt) {
 		case 'f':	/* full_name */
-- 
2.30.2

Re: [PATCH] lib/vsprintf: add __putchar()

[Andy Shevchenko]

On Fri, Aug 27, 2021 at 10:11:55AM -0700, Yury Norov wrote:
> There are 26 occurrences of the code snippet like this in the file :
> 	if (buf < end)
> 	        *buf = separator;
> 	++buf;
> 
> This patch adds a helper function __putchar() to replace opencoding.
> It adds a lot to readability, and also saves 43 bytes of text on x86.

Last time I tried similar it failed the compilation.

Anyway, while you remove a lot of code I'm not sure it makes the code better
to read and understand. Also, we use the same idiom outside of this file.

I would ask Rasmus' opinion on this.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] lib/vsprintf: add __putchar()

[Steven Rostedt]

On Sun, 29 Aug 2021 16:36:13 +0300
Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:

> On Fri, Aug 27, 2021 at 10:11:55AM -0700, Yury Norov wrote:
> > There are 26 occurrences of the code snippet like this in the file :
> > 	if (buf < end)
> > 	        *buf = separator;
> > 	++buf;
> > 
> > This patch adds a helper function __putchar() to replace opencoding.
> > It adds a lot to readability, and also saves 43 bytes of text on x86.  
> 
> Last time I tried similar it failed the compilation.
> 
> Anyway, while you remove a lot of code I'm not sure it makes the code better
> to read and understand. Also, we use the same idiom outside of this file.
> 
> I would ask Rasmus' opinion on this.
> 

I actually like the clean up, although I haven't reviewed the entire patch.

If it is used outside this file, perhaps it should be in a header instead
and those other locations should be updated accordingly.

-- Steve

Re: [PATCH] lib/vsprintf: add __putchar()

[Yury Norov]

On Fri, Sep 03, 2021 at 10:56:07AM -0400, Steven Rostedt wrote:
> On Sun, 29 Aug 2021 16:36:13 +0300
> Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
> 
> > On Fri, Aug 27, 2021 at 10:11:55AM -0700, Yury Norov wrote:
> > > There are 26 occurrences of the code snippet like this in the file :
> > > 	if (buf < end)
> > > 	        *buf = separator;
> > > 	++buf;
> > > 
> > > This patch adds a helper function __putchar() to replace opencoding.
> > > It adds a lot to readability, and also saves 43 bytes of text on x86.  
> > 
> > Last time I tried similar it failed the compilation.
> > 
> > Anyway, while you remove a lot of code I'm not sure it makes the code better
> > to read and understand. Also, we use the same idiom outside of this file.
> > 
> > I would ask Rasmus' opinion on this.
> > 
> 
> I actually like the clean up, although I haven't reviewed the entire patch.

Thanks.
 
> If it is used outside this file, perhaps it should be in a header instead
> and those other locations should be updated accordingly.

I used 'grep "buf < end"' to find spots for cleanup. And except for
lib/vsprintf.c, there is a few random drivers inappropriate for this
cleanup. Andy, can you please share details?

Steve, if you like it, are you OK if I resend this patch? I just found
another spot in lib/vsprintf.c to rework.

Thanks,
Yury

Re: [PATCH] lib/vsprintf: add __putchar()

[Steven Rostedt]

On Fri, 3 Sep 2021 09:01:14 -0700
Yury Norov <yury.norov@gmail.com> wrote:

> 
> Steve, if you like it, are you OK if I resend this patch? I just found
> another spot in lib/vsprintf.c to rework.
> 

You may want to wait to hear from the printk maintainers too.

-- Steve

Re: [PATCH] lib/vsprintf: add __putchar()

[Andy Shevchenko]

On Fri, Sep 3, 2021 at 7:02 PM Yury Norov <yury.norov@gmail.com> wrote:
> On Fri, Sep 03, 2021 at 10:56:07AM -0400, Steven Rostedt wrote:
> > On Sun, 29 Aug 2021 16:36:13 +0300
> > Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
> > > On Fri, Aug 27, 2021 at 10:11:55AM -0700, Yury Norov wrote:

...

> > > > There are 26 occurrences of the code snippet like this in the file :
> > > >   if (buf < end)
> > > >           *buf = separator;
> > > >   ++buf;
> > > >
> > > > This patch adds a helper function __putchar() to replace opencoding.
> > > > It adds a lot to readability, and also saves 43 bytes of text on x86.
> > >
> > > Last time I tried similar it failed the compilation.
> > >
> > > Anyway, while you remove a lot of code I'm not sure it makes the code better
> > > to read and understand. Also, we use the same idiom outside of this file.
> > >
> > > I would ask Rasmus' opinion on this.
> > >
> >
> > I actually like the clean up, although I haven't reviewed the entire patch.
>
> Thanks.
>
> > If it is used outside this file, perhaps it should be in a header instead
> > and those other locations should be updated accordingly.
>
> I used 'grep "buf < end"' to find spots for cleanup.

You need a "smart" grep, i.e. coccinelle for that.

>  And except for
> lib/vsprintf.c, there is a few random drivers inappropriate for this
> cleanup. Andy, can you please share details?

https://elixir.bootlin.com/linux/latest/source/lib/string_helpers.c#L307

-- 
With Best Regards,
Andy Shevchenko