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é 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                           
>   64867     840     592   66299   102fb (TOTALS-lib.a-before)
>   64954     584     588   66126   1024e (TOTALS-lib.a-after)
> and implements some code tidy up.
> 
> Besides reducing lib.a size, it converts many in-tree drivers to use the
> 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.  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: [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é 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
>>   64867     840     592   66299   102fb (TOTALS-lib.a-before)
>>   64954     584     588   66126   1024e (TOTALS-lib.a-after)
>> and implements some code tidy up.
>>
>> Besides reducing lib.a size, it converts many in-tree drivers to use the
>> 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.  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?

Thanks,
André

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é 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: [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é 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.

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