Change in ...coreboot[master]: console: Move poor-man's atoi() into string.h

[Julius Werner (Code Review)]

Hello Patrick Rudolph,

I'd like you to do a code review. Please visit

https://review.coreboot.org/c/coreboot/+/32858

to review the following change.

Change subject: console: Move poor-man's atoi() into string.h
......................................................................

console: Move poor-man's atoi() into string.h

vtxprintf.c seems to have been written before string.h was as fleshed
out as it is today -- this patch removes some custom implementation of
stuff we now have globally. It also moves the skip_atoi() function into
string.h, because I need it somewhere else, and while we maybe don't
want a huge fully-featured string parsing library in coreboot, being
able to parse an integer is occasionally useful.

Change-Id: Iecb2b970aecfc768540d2bf8b3023445f54853a4
Signed-off-by: Julius Werner 
---
M src/console/vtxprintf.c
M src/include/string.h
2 files changed, 14 insertions(+), 16 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/58/32858/1

diff --git a/src/console/vtxprintf.c b/src/console/vtxprintf.c
index f42ed6d..74159d6 100644
--- a/src/console/vtxprintf.c
+++ b/src/console/vtxprintf.c
@@ -24,20 +24,6 @@
 #define SUPPORT_64BIT_INTS
 #endif

-/* haha, don't need ctype.c */
-#define isdigit(c)	((c) >= '0' && (c) <= '9')
-#define is_digit isdigit
-#define isxdigit(c)	(((c) >= '0' && (c) <= '9') || ((c) >= 'a' && (c) <= 'f') || ((c) >= 'A' && (c) <= 'F'))
-
-static int skip_atoi(const char **s)
-{
-	int i = 0;
-
-	while (is_digit(**s))
-		i = i*10 + *((*s)++) - '0';
-	return i;
-}
-
 #define ZEROPAD	1		/* pad with zero */
 #define SIGN	2		/* unsigned/signed long */
 #define PLUS	4		/* show plus */
@@ -175,7 +161,7 @@

/* get field width */
    	field_width = -1;
-		if (is_digit(*fmt)) {
+		if (isdigit(*fmt)) {
    		field_width = skip_atoi(&fmt);
    	} else if (*fmt == '*') {
    		++fmt;
@@ -191,7 +177,7 @@
    	precision = -1;
    	if (*fmt == '.') {
    		++fmt;
-			if (is_digit(*fmt)) {
+			if (isdigit(*fmt)) {
    			precision = skip_atoi(&fmt);
    		} else if (*fmt == '*') {
    			++fmt;
diff --git a/src/include/string.h b/src/include/string.h
index 4a2f5e9..c8de568 100644
--- a/src/include/string.h
+++ b/src/include/string.h
@@ -178,4 +178,16 @@
    	c -= 'A'-'a';
    return c;
 }
+
+/* Parses a positive integer and moves the input pointer forward to the first
+   character that's not a valid digit. */
+static inline int skip_atoi(const char **s)
+{
+	int i = 0;
+
+	while (isdigit(**s))
+		i = i*10 + *((*s)++) - '0';
+	return i;
+}
+
 #endif /* STRING_H */

[Alex Thiessen (Code Review)]

Alex Thiessen has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32858 )

Change subject: console: Move poor-man's atoi() into string.h
......................................................................

Patch Set 3:

(3 comments)

FYI at work, we integrated a small integer parsing library written in lex, featuring negative numbers, hex, octal, binary, digit group separators and detecting overflows. To be published at the end of this year.

https://review.coreboot.org/#/c/32858/3/src/include/string.h 
File src/include/string.h:

https://review.coreboot.org/#/c/32858/3/src/include/string.h@182 
PS3, Line 182: a positive integer
'an integer without a sign' because it works for 0 (not a positive integer) and fails for '+1' (which is one).

https://review.coreboot.org/#/c/32858/3/src/include/string.h@183 
PS3, Line 183:  
Parameter `s` and `*s` must be valid pointers. Resulting value is not specified when an `int` cannot hold enough digits for a given input.

https://review.coreboot.org/#/c/32858/3/src/include/string.h@184 
PS3, Line 184: static inline
Why not put this function to the `string.c`? In 2019, do we still have no LTO enabled in coreboot?

[Julius Werner (Code Review)]

Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32858 )

Change subject: console: Move poor-man's atoi() into string.h
......................................................................

Patch Set 3:

(3 comments)

https://review.coreboot.org/#/c/32858/3/src/include/string.h 
File src/include/string.h:

https://review.coreboot.org/#/c/32858/3/src/include/string.h@182 
PS3, Line 182: a positive integer
> 'an integer without a sign' because it works for 0 (not a positive integer) and fails for '+1' (whic […]

Okay, I'll say unsigned.

https://review.coreboot.org/#/c/32858/3/src/include/string.h@183 
PS3, Line 183:
> Parameter `s` and `*s` must be valid pointers. […]

Added details.

https://review.coreboot.org/#/c/32858/3/src/include/string.h@184 
PS3, Line 184: static inline
> Why not put this function to the `string. […]

Let me do that in a separate CL since it gets a bit more complicated with Makefiles and stuff (and that argument applies to a bunch more functions in here, not just this one).

AFAIK we don't use LTO and LTO is also still not all it's hyped to be in 2019. But there are a bunch of functions here that don't look like they would benefit particularly much from inlining so I think moving them to C code makes sense.

[Julius Werner (Code Review)]

Hello Patrick Rudolph, build bot (Jenkins), Nico Huber,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/32858

to look at the new patch set (#4).

Change subject: console: Move poor-man's atoi() into string.h
......................................................................

console: Move poor-man's atoi() into string.h

vtxprintf.c seems to have been written before string.h was as fleshed
out as it is today -- this patch removes some custom implementation of
stuff we now have globally. It also makes the skip_atoi() function
globally available, because I need it somewhere else, and while we maybe
don't want a huge fully-featured string parsing library in coreboot,
being able to parse an integer is occasionally useful.

Change-Id: Iecb2b970aecfc768540d2bf8b3023445f54853a4
Signed-off-by: Julius Werner 
---
M src/console/vtxprintf.c
M src/include/string.h
2 files changed, 17 insertions(+), 18 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/58/32858/4

[Paul Menzel (Code Review)]

Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32858 )

Change subject: console: Move poor-man's atoi() into string.h
......................................................................

Patch Set 4:

(1 comment)

https://review.coreboot.org/#/c/32858/4/src/include/string.h 
File src/include/string.h:

https://review.coreboot.org/#/c/32858/4/src/include/string.h@182 
PS4, Line 182: /* Parses an unsigned integer and moves the input pointer forward to the first
             :    character that's not a valid digit. s and *s must not be NULL. Result
             :    undefined if it overruns the return type size. */
I though for headers the elaborate multi-line comment style (and not concise) should be used.

[Alex Thiessen (Code Review)]

Alex Thiessen has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32858 )

Change subject: console: Move poor-man's atoi() into string.h
......................................................................

Patch Set 4: Code-Review+1

(3 comments)

I agree with Paul that the comment formatting needs polishing.

https://review.coreboot.org/#/c/32858/3/src/include/string.h 
File src/include/string.h:

https://review.coreboot.org/#/c/32858/3/src/include/string.h@182 
PS3, Line 182: a positive integer
> Okay, I'll say unsigned.

Done

https://review.coreboot.org/#/c/32858/3/src/include/string.h@183 
PS3, Line 183:
> Added details.

Done

https://review.coreboot.org/#/c/32858/3/src/include/string.h@184 
PS3, Line 184: static inline
> Let me do that in a separate CL since it gets a bit more complicated with Makefiles and stuff (and t […]

It's clear that LTO doesn't have a huge impact on boot time or code size. Yet, it enables more freedom for the toolchain regarding inlining and removes a need to put definitions of functions into headers for them to be suitable for inlining.

Sure, Identical Code Folding is supposed to collapse all the instantiations of such header-defined static functions. It's ugly but acceptable.

[Julius Werner (Code Review)]

Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32858 )

Change subject: console: Move poor-man's atoi() into string.h
......................................................................

Patch Set 4:

(2 comments)

https://review.coreboot.org/#/c/32858/4/src/include/string.h 
File src/include/string.h:

https://review.coreboot.org/#/c/32858/4/src/include/string.h@182 
PS4, Line 182: /* Parses an unsigned integer and moves the input pointer forward to the first
             :    character that's not a valid digit. s and *s must not be NULL. Result
             :    undefined if it overruns the return type size. */
> I though for headers the elaborate multi-line comment style (and not concise) should be used.

I am not aware of any such rule, AFAIR it's just about length and whether it's a single comment that just happened to go over the line or an elaborate multi-paragraph document. But I can change it here if you prefer.

https://review.coreboot.org/#/c/32858/3/src/include/string.h 
File src/include/string.h:

https://review.coreboot.org/#/c/32858/3/src/include/string.h@184 
PS3, Line 184: static inline
> It's clear that LTO doesn't have a huge impact on boot time or code size. […]

What I was trying to say is that I don't think that an external function linked with LTO can always be optimized as much as an inline. AFAIK current LTO implementations are still way too limited for that (especially in cases where large parts of the function essentially constant-fold away into nothing). But I'm not an expert on the topic so I might be wrong.

Regardless, as far as I'm aware coreboot isn't using it atm (maybe someone should look into that?).

[Julius Werner (Code Review)]

Hello Alex Thiessen, Patrick Rudolph, build bot (Jenkins), Nico Huber,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/32858

to look at the new patch set (#5).

Change subject: console: Move poor-man's atoi() into string.h
......................................................................

console: Move poor-man's atoi() into string.h

vtxprintf.c seems to have been written before string.h was as fleshed
out as it is today -- this patch removes some custom implementation of
stuff we now have globally. It also makes the skip_atoi() function
globally available, because I need it somewhere else, and while we maybe
don't want a huge fully-featured string parsing library in coreboot,
being able to parse an integer is occasionally useful.

Change-Id: Iecb2b970aecfc768540d2bf8b3023445f54853a4
Signed-off-by: Julius Werner 
---
M src/console/vtxprintf.c
M src/include/string.h
2 files changed, 19 insertions(+), 18 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/58/32858/5

[Alex Thiessen (Code Review)]

Alex Thiessen has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32858 )

Change subject: console: Move poor-man's atoi() into string.h
......................................................................

Patch Set 5: Code-Review+2

[Patrick Georgi (Code Review)]

Patrick Georgi has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/32858 )

Change subject: console: Move poor-man's atoi() into string.h
......................................................................

console: Move poor-man's atoi() into string.h

vtxprintf.c seems to have been written before string.h was as fleshed
out as it is today -- this patch removes some custom implementation of
stuff we now have globally. It also makes the skip_atoi() function
globally available, because I need it somewhere else, and while we maybe
don't want a huge fully-featured string parsing library in coreboot,
being able to parse an integer is occasionally useful.

Change-Id: Iecb2b970aecfc768540d2bf8b3023445f54853a4
Signed-off-by: Julius Werner 
Reviewed-on: https://review.coreboot.org/c/coreboot/+/32858
Tested-by: build bot (Jenkins) 
Reviewed-by: Alex Thiessen 
---
M src/console/vtxprintf.c
M src/include/string.h
2 files changed, 19 insertions(+), 18 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Alex Thiessen: Looks good to me, approved

diff --git a/src/console/vtxprintf.c b/src/console/vtxprintf.c
index f42ed6d..c429ac7 100644
--- a/src/console/vtxprintf.c
+++ b/src/console/vtxprintf.c
@@ -24,20 +24,6 @@
 #define SUPPORT_64BIT_INTS
 #endif

-/* haha, don't need ctype.c */
-#define isdigit(c)	((c) >= '0' && (c) <= '9')
-#define is_digit isdigit
-#define isxdigit(c)	(((c) >= '0' && (c) <= '9') || ((c) >= 'a' && (c) <= 'f') || ((c) >= 'A' && (c) <= 'F'))
-
-static int skip_atoi(const char **s)
-{
-	int i = 0;
-
-	while (is_digit(**s))
-		i = i*10 + *((*s)++) - '0';
-	return i;
-}
-
 #define ZEROPAD	1		/* pad with zero */
 #define SIGN	2		/* unsigned/signed long */
 #define PLUS	4		/* show plus */
@@ -175,8 +161,8 @@

/* get field width */
    	field_width = -1;
-		if (is_digit(*fmt)) {
-			field_width = skip_atoi(&fmt);
+		if (isdigit(*fmt)) {
+			field_width = skip_atoi((char **)&fmt);
    	} else if (*fmt == '*') {
    		++fmt;
    		/* it's the next argument */
@@ -191,8 +177,8 @@
    	precision = -1;
    	if (*fmt == '.') {
    		++fmt;
-			if (is_digit(*fmt)) {
-				precision = skip_atoi(&fmt);
+			if (isdigit(*fmt)) {
+				precision = skip_atoi((char **)&fmt);
    		} else if (*fmt == '*') {
    			++fmt;
    			/* it's the next argument */
diff --git a/src/include/string.h b/src/include/string.h
index 4a2f5e9..c56a760 100644
--- a/src/include/string.h
+++ b/src/include/string.h
@@ -178,4 +178,19 @@
    	c -= 'A'-'a';
    return c;
 }
+
+/*
+ * Parses an unsigned integer and moves the input pointer forward to the first
+ * character that's not a valid digit. s and *s must not be NULL. Result
+ * undefined if it overruns the return type size.
+ */
+static inline unsigned int skip_atoi(char **s)
+{
+	unsigned int i = 0;
+
+	while (isdigit(**s))
+		i = i*10 + *((*s)++) - '0';
+	return i;
+}
+
 #endif /* STRING_H */