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

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

[James Bottomley]

On Sat, 2009-11-07 at 13:16 -0200, Andr=C3=A9 Goddard Rosa wrote:
> This patch reduces lib.a code size by 173 bytes on my Core 2 with gcc 4=
.4.1
> even considering that it exports a newly defined function skip_spaces()
> to drivers:
>    text    data     bss     dec     hex filename                       =
   =20
>   64867     840     592   66299   102fb (TOTALS-lib.a-before)
>   64954     584     588   66126   1024e (TOTALS-lib.a-after)
> and implements some code tidy up.
>=20
> Besides reducing lib.a size, it converts many in-tree drivers to use th=
e
> newly defined function, which makes another small reduction on kernel s=
ize
> overall when those drivers are used.

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.

Assuming there's a good answer to the above:

> + * 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)

I don't think const return is a good idea because most functions will be
manipulating the string and using pointers that won't be const, so this
will generate a ton of 'initialization discards qualifiers from pointer
target type' ... so that leads to the question of whether this patch
series was actually compiled ...

James

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

[Alan Cox]

On Sat,  7 Nov 2009 13:16:18 -0200
Andr=E9 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.

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().

Alan

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

[André Goddard Rosa]

Hi, James!

On Sun, Nov 8, 2009 at 2:05 PM, James Bottomley
<James.Bottomley@hansenpartnership.com> wrote:
> On Sat, 2009-11-07 at 13:16 -0200, Andr=E9 Goddard Rosa wrote:
>> This patch reduces lib.a code size by 173 bytes on my Core 2 with gcc =
4.4.1
>> even considering that it exports a newly defined function skip_spaces(=
)
>> to drivers:
>> =A0 =A0text =A0 =A0data =A0 =A0 bss =A0 =A0 dec =A0 =A0 hex filename
>> =A0 64867 =A0 =A0 840 =A0 =A0 592 =A0 66299 =A0 102fb (TOTALS-lib.a-be=
fore)
>> =A0 64954 =A0 =A0 584 =A0 =A0 588 =A0 66126 =A0 1024e (TOTALS-lib.a-af=
ter)
>> and implements some code tidy up.
>>
>> Besides reducing lib.a size, it converts many in-tree drivers to use t=
he
>> newly defined function, which makes another small reduction on kernel =
size
>> overall when those drivers are used.
>
> Before we embark on something as massive as this, could we take a step
> back. =A0I 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. =A0I 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?

Thanks,
Andr=E9

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

[Theodore Tso]

On Sat, Nov 07, 2009 at 01:16:20PM -0200, Andr=E9 Goddard Rosa wrote:
> Makes use of skip_spaces() defined in lib/string.c for removing leading
> spaces from strings all over the tree.
>=20
> 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)) als=
o
> evaluates to 0 whenever *str =3D=3D 0, making it redundant. In other wo=
rds,
> "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.

						- Ted

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

[Julia Lawall]

> > 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

diff -u -p a/drivers/leds/led-class.c b/drivers/leds/led-class.c
--- a/drivers/leds/led-class.c
+++ b/drivers/leds/led-class.c
@@ -50,7 +50,7 @@ static ssize_t led_brightness_store(stru
 	unsigned long state = simple_strtoul(buf, &after, 10);
 	size_t count = after - buf;
 
-	if (*after && isspace(*after))
+	if (isspace(*after))
 		count++;
 
 	if (count == size) {
diff -u -p a/drivers/leds/ledtrig-timer.c b/drivers/leds/ledtrig-timer.c
--- a/drivers/leds/ledtrig-timer.c
+++ b/drivers/leds/ledtrig-timer.c
@@ -83,7 +83,7 @@ static ssize_t led_delay_on_store(struct
 	unsigned long state = simple_strtoul(buf, &after, 10);
 	size_t count = after - buf;
 
-	if (*after && isspace(*after))
+	if (isspace(*after))
 		count++;
 
 	if (count == size) {
@@ -127,7 +127,7 @@ static ssize_t led_delay_off_store(struc
 	unsigned long state = simple_strtoul(buf, &after, 10);
 	size_t count = after - buf;
 
-	if (*after && isspace(*after))
+	if (isspace(*after))
 		count++;
 
 	if (count == size) {
diff -u -p a/drivers/video/backlight/lcd.c b/drivers/video/backlight/lcd.c
--- a/drivers/video/backlight/lcd.c
+++ b/drivers/video/backlight/lcd.c
@@ -101,7 +101,7 @@ static ssize_t lcd_store_power(struct de
 	int power = simple_strtoul(buf, &endp, 0);
 	size_t size = endp - buf;
 
-	if (*endp && isspace(*endp))
+	if (isspace(*endp))
 		size++;
 	if (size != count)
 		return -EINVAL;
@@ -140,7 +140,7 @@ static ssize_t lcd_store_contrast(struct
 	int contrast = simple_strtoul(buf, &endp, 0);
 	size_t size = endp - buf;
 
-	if (*endp && isspace(*endp))
+	if (isspace(*endp))
 		size++;
 	if (size != count)
 		return -EINVAL;
diff -u -p a/drivers/video/display/display-sysfs.c b/drivers/video/display/display-sysfs.c
--- a/drivers/video/display/display-sysfs.c
+++ b/drivers/video/display/display-sysfs.c
@@ -67,7 +67,7 @@ static ssize_t display_store_contrast(st
 	contrast = simple_strtoul(buf, &endp, 0);
 	size = endp - buf;
 
-	if (*endp && isspace(*endp))
+	if (isspace(*endp))
 		size++;
 
 	if (size != count)
diff -u -p a/drivers/video/output.c b/drivers/video/output.c
--- a/drivers/video/output.c
+++ b/drivers/video/output.c
@@ -50,7 +50,7 @@ static ssize_t video_output_store_state(
 	int request_state = simple_strtoul(buf,&endp,0);
 	size_t size = endp - buf;
 
-	if (*endp && isspace(*endp))
+	if (isspace(*endp))
 		size++;
 	if (size != count)
 		return -EINVAL;