2009-11-08 16:05:03

by James Bottomley

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

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


2009-11-08 16:52:18

by André Goddard Rosa

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

Hi, James!

On Sun, Nov 8, 2009 at 2:05 PM, James Bottomley
<[email protected]> 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?