[PATCH v4 10/12] string: factorize skip_spaces and export it to be generally available

[PATCH v4 10/12] string: factorize skip_spaces and export it to be generally available

[André Goddard Rosa]

On the following sentence:
    while (*s && isspace(*s))
        s++;

If *s == 0, isspace() evaluates to ((_ctype[*s] & 0x20) != 0), which
evaluates to ((0x08 & 0x20) != 0) which equals to 0 as well.
If *s == 1, we depend on isspace() result anyway.

In other words, "a char equals zero is never a space". So remove this check.
Also, *s != 0 is by far the most common case (non-empty string).

Fixed const return as noticed by Jan Engelhardt and James Bottomley

Signed-off-by: André Goddard Rosa <andre.goddard@gmail.com>
---
 include/linux/string.h |    1 +
 lib/string.c           |   19 +++++++++++++++----
 2 files changed, 16 insertions(+), 4 deletions(-)

diff --git a/include/linux/string.h b/include/linux/string.h
index b850886..3bba9ee 100644
--- a/include/linux/string.h
+++ b/include/linux/string.h
@@ -62,6 +62,7 @@ extern char * strnchr(const char *, size_t, int);
 #ifndef __HAVE_ARCH_STRRCHR
 extern char * strrchr(const char *,int);
 #endif
+extern char * __must_check skip_spaces(const char *);
 extern char * __must_check strstrip(char *);
 #ifndef __HAVE_ARCH_STRSTR
 extern char * strstr(const char *,const char *);
diff --git a/lib/string.c b/lib/string.c
index b19b87a..d9a51d5 100644
--- a/lib/string.c
+++ b/lib/string.c
@@ -330,6 +330,20 @@ EXPORT_SYMBOL(strnchr);
 #endif
 
 /**
+ * skip_spaces - Removes leading whitespace from @s.
+ * @s: The string to be stripped.
+ *
+ * Returns a pointer to the first non-whitespace character in @s.
+ */
+char *skip_spaces(const char *str)
+{
+	while (isspace(*str))
+		++str;
+	return str;
+}
+EXPORT_SYMBOL(skip_spaces);
+
+/**
  * strstrip - Removes leading and trailing whitespace from @s.
  * @s: The string to be stripped.
  *
@@ -352,10 +366,7 @@ char *strstrip(char *s)
 		end--;
 	*(end + 1) = '\0';
 
-	while (*s && isspace(*s))
-		s++;
-
-	return s;
+	return (char *)skip_spaces(s);
 }
 EXPORT_SYMBOL(strstrip);
 
-- 
1.6.5.2.153.g6e31f.dirty

Re: [PATCH v4 10/12] string: factorize skip_spaces and export it to be generally available

[Jan Engelhardt]

On Saturday 2009-11-07 16:16, André Goddard Rosa wrote:

> /**
>+ * skip_spaces - Removes leading whitespace from @s.
>+ * @s: The string to be stripped.
>+ *
>+ * Returns a pointer to the first non-whitespace character in @s.
>+ */
>+const char *skip_spaces(const char *str)
>+{
>+	while (isspace(*str))
>+		++str;
>+	return str;
>+}
>+EXPORT_SYMBOL(skip_spaces);
>+

I would make this
	char *skip_spaces(const char *)

just like most of the stdc functions, so that you do not need
ugly casts like this (v) in callers of skip_spaces.

>-	while (*s && isspace(*s))
>-		s++;
>-
>-	return s;
>+	return (char *)skip_spaces(s);
> }
> EXPORT_SYMBOL(strstrip);

Re: [PATCH v4 10/12] string: factorize skip_spaces and export it to be generally available

[James Bottomley]

On Sat, 2009-11-07 at 14:23 -0200, André Goddard Rosa wrote:
> + * skip_spaces - Removes leading whitespace from @s.
> + * @s: The string to be stripped.
> + *
> + * Returns a pointer to the first non-whitespace character in @s.
> + */
> +char *skip_spaces(const char *str)

OK, so this now becomes a bad interface because it silently promotes
const to non const ... and I'm sure the compiler would warn about this
too in string.c ... did this get compiled?

James

Re: [PATCH v4 10/12] string: factorize skip_spaces and export it to be generally available

[Jan Engelhardt]

On Sunday 2009-11-08 17:38, James Bottomley wrote:

>On Sat, 2009-11-07 at 14:23 -0200, André Goddard Rosa wrote:
>> + * skip_spaces - Removes leading whitespace from @s.
>> + * @s: The string to be stripped.
>> + *
>> + * Returns a pointer to the first non-whitespace character in @s.
>> + */
>> +char *skip_spaces(const char *str)
>
>OK, so this now becomes a bad interface because it silently promotes
>const to non const ...

strchr, etc. all do the same. Do you think they are bad too?

Re: [PATCH v4 10/12] string: factorize skip_spaces and export it to be generally available

[André Goddard Rosa]

On Sun, Nov 8, 2009 at 2:54 PM, Jan Engelhardt <jengelh@medozas.de> wrote:
>
> On Sunday 2009-11-08 17:38, James Bottomley wrote:
>
>>On Sat, 2009-11-07 at 14:23 -0200, André Goddard Rosa wrote:
>>> + * skip_spaces - Removes leading whitespace from @s.
>>> + * @s: The string to be stripped.
>>> + *
>>> + * Returns a pointer to the first non-whitespace character in @s.
>>> + */
>>> +char *skip_spaces(const char *str)
>>
>>OK, so this now becomes a bad interface because it silently promotes
>>const to non const ...
>
> strchr, etc. all do the same. Do you think they are bad too?

Yeap, I just based on those.

James, what would you suggest instead?

Thanks,
André

Re: [PATCH v4 10/12] string: factorize skip_spaces and export it to be generally available

[Jan Engelhardt]

On Saturday 2009-11-07 17:30, André Goddard Rosa wrote:

>+char *skip_spaces(const char *str)

>@@ -352,10 +366,7 @@ char *strstrip(char *s)
> 		end--;
> 	*(end + 1) = '\0';
> 
>-	while (*s && isspace(*s))
>-		s++;
>-
>-	return s;
>+	return (char *)skip_spaces(s);
> }

sparse would probably say "how much more casting to the same type do you want?"
It is already char*.

Re: [PATCH v4 10/12] string: factorize skip_spaces and export it to be generally available

[André Goddard Rosa]

On Sun, Nov 8, 2009 at 2:50 PM, Alan Cox <alan@lxorguk.ukuu.org.uk> wrote:
> On Sat,  7 Nov 2009 13:16:18 -0200
> André Goddard Rosa <andre.goddard@gmail.com> wrote:
>
>> On the following sentence:
>>     while (*s && isspace(*s))
>>         s++;
>
> Looks fine but for one thing: it's actually shorter inline than moved
> into /lib so at the very least it should be a header inline not a
> function call.

I have tried header "static inline" approach per your suggestion and
code size increases by 197 bytes from:
64954     584     588   66126   1024e (TOTALS-lib.a-before)
to:
65151     584     588   66323   10313 (TOTALS-lib.a-after)

> Second minor comment. Although it never made it into the final ANSI C,
> the proposed name (and the one used in a lot of other non Linux code for
> this) is stpblk().

Thank you,
André

[PATCH v4 08/12] vsprintf: reuse almost identical simple_strtoulX() functions

[André Goddard Rosa]

The difference between simple_strtoul() and simple_strtoull() is just
the size of the variable used to keep track of the sum of characters
converted to numbers:

unsigned long simple_strtoul() {...}
unsigned long long simple_strtoull(){...}

Both are same size (8 bytes) on my Core 2/gcc 4.4.1.
Overflow condition is not checked on both functions, so an extremely large
string can break these functions so that they don't even notice it.

As we do not care for overflowing on these functions, always keep the sum
using the larger variable around (unsigned long long) on simple_strtoull()
and cast it to (unsigned long) on simple_strtoul(), which then becomes
just a wrapper around simple_strtoull().

Code size decreases by 304 bytes:
   text    data     bss     dec     hex filename
  15543       0       8   15551    3cbf lib/vsprintf.o-before
  15239       0       8   15247    3b8f lib/vsprintf.o-after

Removed unnecessary cast as noticed by Jan Engelhardt.

Signed-off-by: André Goddard Rosa <andre.goddard@gmail.com>
Acked-by: Ingo Molnar <mingo@elte.hu>
---
 lib/vsprintf.c |   48 ++++++++++++++----------------------------------
 1 files changed, 14 insertions(+), 34 deletions(-)

diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 566c947..7ec96a3 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -49,14 +49,14 @@ static unsigned int simple_guess_base(const char *cp)
 }
 
 /**
- * simple_strtoul - convert a string to an unsigned long
+ * simple_strtoull - convert a string to an unsigned long long
  * @cp: The start of the string
  * @endp: A pointer to the end of the parsed string will be placed here
  * @base: The number base to use
  */
-unsigned long simple_strtoul(const char *cp, char **endp, unsigned int base)
+unsigned long long simple_strtoull(const char *cp, char **endp, unsigned int base)
 {
-	unsigned long result = 0;
+	unsigned long long result = 0;
 
 	if (!base)
 		base = simple_guess_base(cp);
@@ -78,54 +78,34 @@ unsigned long simple_strtoul(const char *cp, char **endp, unsigned int base)
 
 	return result;
 }
-EXPORT_SYMBOL(simple_strtoul);
+EXPORT_SYMBOL(simple_strtoull);
 
 /**
- * simple_strtol - convert a string to a signed long
+ * simple_strtoul - convert a string to an unsigned long
  * @cp: The start of the string
  * @endp: A pointer to the end of the parsed string will be placed here
  * @base: The number base to use
  */
-long simple_strtol(const char *cp, char **endp, unsigned int base)
+unsigned long simple_strtoul(const char *cp, char **endp, unsigned int base)
 {
-	if (*cp == '-')
-		return -simple_strtoul(cp + 1, endp, base);
-
-	return simple_strtoul(cp, endp, base);
+	return simple_strtoull(cp, endp, base);
 }
-EXPORT_SYMBOL(simple_strtol);
+EXPORT_SYMBOL(simple_strtoul);
 
 /**
- * simple_strtoull - convert a string to an unsigned long long
+ * simple_strtol - convert a string to a signed long
  * @cp: The start of the string
  * @endp: A pointer to the end of the parsed string will be placed here
  * @base: The number base to use
  */
-unsigned long long simple_strtoull(const char *cp, char **endp, unsigned int base)
+long simple_strtol(const char *cp, char **endp, unsigned int base)
 {
-	unsigned long long result = 0;
-
-	if (!base)
-		base = simple_guess_base(cp);
-
-	if (base == 16 && cp[0] == '0' && TOLOWER(cp[1]) == 'x')
-		cp += 2;
-
-	while (isxdigit(*cp)) {
-		unsigned int value;
-
-		value = isdigit(*cp) ? *cp - '0' : TOLOWER(*cp) - 'a' + 10;
-		if (value >= base)
-			break;
-		result = result * base + value;
-		cp++;
-	}
-	if (endp)
-		*endp = (char *)cp;
+	if (*cp == '-')
+		return -simple_strtoul(cp + 1, endp, base);
 
-	return result;
+	return simple_strtoul(cp, endp, base);
 }
-EXPORT_SYMBOL(simple_strtoull);
+EXPORT_SYMBOL(simple_strtol);
 
 /**
  * simple_strtoll - convert a string to a signed long long
-- 
1.6.5.2.153.g6e31f.dirty

Re: [PATCH v4 01/12] vsprintf: factorize "(null)" string

[Jan Engelhardt]

Stripping the humongous Cc list for sanity.


On Saturday 2009-11-07 16:16, André Goddard Rosa wrote:

>Change "<NULL>" to "(null)" and make it a static const char[] hoping that
>the compiler will make null_str a label to a read-only area containing it.

Hoping? Nah, thanks.

>See:
>http://udrepper.livejournal.com/13851.html

Ulrich's example already _has_ a variable that is then changed from 
const char * to const char[]. Of course doing that will save you the 
extra pointer.

But vsprintf.c on the other hand did not have that extra variable to 
begin with! But it is ok nevertheless, and the unification of <NULL> vs 
(null) is worthwhile.

Re: [PATCH v4 01/12] vsprintf: factorize "(null)" string

[André Goddard Rosa]

On Sun, Nov 8, 2009 at 1:37 PM, Jan Engelhardt <jengelh@medozas.de> wrote:
>
> Stripping the humongous Cc list for sanity.
>
>
> On Saturday 2009-11-07 16:16, André Goddard Rosa wrote:
>
>>Change "<NULL>" to "(null)" and make it a static const char[] hoping that
>>the compiler will make null_str a label to a read-only area containing it.
>
> Hoping? Nah, thanks.

:) That's just because I don't always trust gcc blindly. :)

>>See:
>>http://udrepper.livejournal.com/13851.html
>
> Ulrich's example already _has_ a variable that is then changed from
> const char * to const char[]. Of course doing that will save you the
> extra pointer.
> But vsprintf.c on the other hand did not have that extra variable to
> begin with!

Correct, of course! Most relevant reference is the first one:
http://people.redhat.com/drepper/dsohowto.pdf part 2.4.2

The others are just related/akin to the topic; interesting nevertheless.

> But it is ok nevertheless, and the unification of <NULL> vs
> (null) is worthwhile.

Thanks,
André

Re: [PATCH v4 01/12] vsprintf: factorize "(null)" string

[Rusty Russell]

On Sun, 8 Nov 2009 01:46:09 am you wrote:
> Change "<NULL>" to "(null)" and make it a static const char[] hoping that
> the compiler will make null_str a label to a read-only area containing it.

(Trimmed the 79 recipients of the original).

Hi Andre,

Consistently using <NULL> or (null) makes sense.  But I'm really missing your argument: is there some reason why normal string merging won't work?

Confused,
Rusty.

Re: [PATCH v4 01/12] vsprintf: factorize "(null)" string

[André Goddard Rosa]

On Mon, Nov 9, 2009 at 1:28 AM, Rusty Russell <rusty@rustcorp.com.au> wrote:
> Hi Andre,
>
> Consistently using <NULL> or (null) makes sense.  But I'm really missing your argument: is there some reason
> why normal string merging won't work?
>

Hi, Rusty!

    I agree that it works fine as well; the factorization only makes
explicit the fact that's a constant string, that's all.

Thank you,
André

Re: [PATCH v4 07/12] vsprintf: factor out skip_space code in a separate function

[Jan Engelhardt]

On Saturday 2009-11-07 16:16, André Goddard Rosa wrote:

>It decreases code size:

And may increase runtime. If GCC determined that skip_space
itself (without the noinline you added) is going to be inlined,
let it go ahead.

>   text    data     bss     dec     hex filename
>  15719       0       8   15727    3d6f lib/vsprintf.o-before
>  15543       0       8   15551    3cbf lib/vsprintf.o-after

Re: [PATCH v4 07/12] vsprintf: factor out skip_space code in a separate function

[André Goddard Rosa]

On Sun, Nov 8, 2009 at 1:44 PM, Jan Engelhardt <jengelh@medozas.de> wrote:
>
> On Saturday 2009-11-07 16:16, André Goddard Rosa wrote:
>
>>It decreases code size:
>
> And may increase runtime. If GCC determined that skip_space
> itself (without the noinline you added) is going to be inlined,
> let it go ahead.

Later in the series, when stepping over the whole tree for using that function,
I changed it to let gcc decide instead as you correctly pointed out.

Thanks for reviewing,
André

Re:

[André Goddard Rosa]

On Mon, Nov 9, 2009 at 10:39 AM, David Howells <dhowells@redhat.com> wrote:
> MIME-Version: 1.0
> Content-Type: text/plain; charset=iso-8859-1
> Content-Transfer-Encoding: quoted-printable
>
> Andr=E9 Goddard Rosa <andre.goddard@gmail.com> wrote:
>
>> It decreases code size:
>>    text    data     bss     dec     hex filename
>>   15719       0       8   15727    3d6f lib/vsprintf.o-before
>>   15543       0       8   15551    3cbf lib/vsprintf.o-after
>
> Whilst this may be true, there will be a countervailing decrease in
> performance.  Have you assessed that?

(trimmed long cc: list to keep it sane, I'll not use get_maintainer.pl
output this way anymore)

I'm not sure it decreases performance. From the last iteration of the
patch, I removed
the hint to force "not inlining", so that gcc can inline it if it
thinks it's better.

Are those so performance sensitive that it makes sense to perform this
assessment?
If you think so, what would you suggest? A micro-benchmark or some
real use case?

Best regards,
André

Re: [dm-devel] [PATCH v4 00/12] introduce skip_spaces(), reducing code size plus some clean-ups

[Jan Engelhardt]

On Sunday 2009-11-08 17:52, André Goddard Rosa wrote:

>Hi, James!
>
>On Sun, Nov 8, 2009 at 2:05 PM, James Bottomley
><James.Bottomley@hansenpartnership.com> wrote:
>> Before we embark on something as massive as this, could we take a step
>> back.  I agree that if I were coming up with the strstip() interface
>> today I probably wouldn't have given it two overloaded uses.
>>
>> However, I think the function, in spite of this minor issue, is very
>> usable.  I still don't understand why people thought adding a
>> __must_check, which is what damaged one of the overloaded uses, is a
>> good idea.
>
>Differently of "static void strip(char *str)"@scripts/kconfig/conf.c ,
>this function
>does not moves the characters to the beginning of the string, so that if that
>string is going to be reused it should refer to the newly returned string start.
>
>I've changed it to remove the const and return a "char *".
>
>Do you think __must_check is not needed as well?

If you called strstrip, and not use its result, what good would that
do besides being effectively a strrtrim? __must_check is a good thing IMO.

Re: [PATCH v4 00/12] introduce skip_spaces(), reducing code size plus some clean-ups

[Alexey Dobriyan]

On 11/7/09, André Goddard Rosa <andre.goddard@gmail.com> wrote:
> skip_spaces()

This is pointless wrapper and if you don't understand and immediately see
what such code does you shouldn't be allowed near kernel.

"const _ctype" patch should be fine.

It's pretty sad to see so much activity around all sorts of simple and
simultaneously
irrelevant stuff like formatting, printk, pr_debug, printk_once, loglevels etc

I suggest to kernel newbies to find and fix a bug. You'll learn much
more from this exercise.

Re: [ibm-acpi-devel] [PATCH v4 12/12] tree-wide: convert open calls to remove spaces to skip_spaces() lib function

[Henrique de Moraes Holschuh]

On Sat, 07 Nov 2009, André Goddard Rosa wrote:
> Also, while at it, if we see (*str && isspace(*str)), we can be sure to
> remove the first condition (*str) as the second one (isspace(*str)) also
> evaluates to 0 whenever *str == 0, making it redundant. In other words,
> "a char equals zero is never a space".

You didn't document in isspace() that it must return false for NUL.  Please
do that before you remove any such checks.

-- 
  "One disk to rule them all, One disk to find them. One disk to bring
  them all and in the darkness grind them. In the Land of Redmond
  where the shadows lie." -- The Silicon Valley Tarot
  Henrique Holschuh

Re: [ibm-acpi-devel] [PATCH v4 12/12] tree-wide: convert open calls to remove spaces to skip_spaces() lib function

[André Goddard Rosa]

On Sat, Nov 14, 2009 at 1:20 PM, Henrique de Moraes Holschuh
<hmh@hmh.eng.br> wrote:
> On Sat, 07 Nov 2009, André Goddard Rosa wrote:
>> Also, while at it, if we see (*str && isspace(*str)), we can be sure to
>> remove the first condition (*str) as the second one (isspace(*str)) also
>> evaluates to 0 whenever *str == 0, making it redundant. In other words,
>> "a char equals zero is never a space".
>
> You didn't document in isspace() that it must return false for NUL.  Please
> do that before you remove any such checks.
>

Good idea Henrique, I will do it!

Thank you,
André

Re: [PATCH v4 12/12] tree-wide: convert open calls to remove spaces to skip_spaces() lib function

[André Goddard Rosa]

[stripped very long cc: line]

On Sun, Nov 8, 2009 at 6:23 PM, Julia Lawall <julia@diku.dk> wrote:
>> > Also, while at it, if we see (*str && isspace(*str)), we can be sure to
>> > remove the first condition (*str) as the second one (isspace(*str)) also
>> > evaluates to 0 whenever *str == 0, making it redundant. In other words,
>> > "a char equals zero is never a space".
>
> I tried the following semantic patch (http://coccinelle.lip6.fr), and got
> the results below.
>
> @@
> expression str;
> @@
>
> ( // ignore skip_spaces cases
>  while (*str &&  isspace(*str)) { \(str++;\|++str;\) }
> |
> - *str &&
>  isspace(*str)
> )
>
> I haven't checked the results in any way, however.
>
> julia
>

Hi, Julia!

    Your patch looks good, but it conflicted at the following files
when I tried to apply it:

> diff -u -p a/drivers/video/backlight/lcd.c b/drivers/video/backlight/lcd.c
> diff -u -p a/drivers/video/display/display-sysfs.c b/drivers/video/display/display-sysfs.c

    That's because I had changed them as well.
    The other ones you spotted I haven't seen originally:

> diff -u -p a/drivers/leds/led-class.c b/drivers/leds/led-class.c
> diff -u -p a/drivers/leds/ledtrig-timer.c b/drivers/leds/ledtrig-timer.c
> diff -u -p a/drivers/video/output.c b/drivers/video/output.c

    Do you mind sending another patch or do you want me folding these in?

Thank you,
André

Re: [PATCH v4 12/12] tree-wide: convert open calls to remove spaces to skip_spaces() lib function

[André Goddard Rosa]

[trimmed very long cc: line]

On Sun, Nov 8, 2009 at 4:47 PM, Theodore Tso <tytso@mit.edu> wrote:
> On Sat, Nov 07, 2009 at 01:16:20PM -0200, André Goddard Rosa wrote:
>> Makes use of skip_spaces() defined in lib/string.c for removing leading
>> spaces from strings all over the tree.
>>
>> Also, while at it, if we see (*str && isspace(*str)), we can be sure to
>> remove the first condition (*str) as the second one (isspace(*str)) also
>> evaluates to 0 whenever *str == 0, making it redundant. In other words,
>> "a char equals zero is never a space".
>
> There are a number of places that have the pattern of skipping
> whitespace, calling simpler_strtoul(), and then skipping whitespace
> afterwards.  And thinkpad_acpi.c and fs/ext4/super.c both have an
> indentical function, parse_strotul(), which basically does this plus
> doing actual error checking (a number of callers of simple_strtoul
> aren't checking to see if the user passed in a valid number or not,
> boo.)
>
> I would suggest that we should lift parse_strtoul() into lib/, both to
> save a bit of code, as well as encouraging people to do proper input
> validation, while we are doing this tree-wide cleanup.
>

Hi, Ted!

    I took a look at it but couldn't find use for it much besides the
ones you've
pointed out, so I'm not sure it really deserves a lib function. Most calls to
simple_strtoul() does not skips spaces.

Thanks,
André