[PATCH] shrink last_char_is function even more

Tito farmatito at tiscali.it
Sun Jul 19 20:31:09 UTC 2020



On 7/19/20 8:48 PM, Denys Vlasenko wrote:
> On Tue, Jul 7, 2020 at 9:17 PM Tito <farmatito at tiscali.it> wrote:
>> Hi,
>> attached you will find a patch that shrinks libbb's last_char_is function.
>> bloatcheck is:
>>
>> function                                             old     new   delta
>> last_char_is                                          53      42     -11
>> ------------------------------------------------------------------------------
>> (add/remove: 0/0 grow/shrink: 0/1 up/down: 0/-11)             Total: -11 bytes
>>    text    data     bss     dec     hex filename
>>  981219   16907    1872  999998   f423e busybox_old
>>  981208   16907    1872  999987   f4233 busybox_unstripped
>>
>> Patch for review:
>>
>> --- libbb/last_char_is.c.orig   2020-07-05 09:54:24.737931000 +0200
>> +++ libbb/last_char_is.c        2020-07-06 14:29:27.768898574 +0200
>> @@ -11,14 +11,7 @@
>>  /* Find out if the last character of a string matches the one given */
>>  char* FAST_FUNC last_char_is(const char *s, int c)
>>  {
>> -       if (s) {
>> -               size_t sz = strlen(s);
>> -               /* Don't underrun the buffer if the string length is 0 */
>> -               if (sz != 0) {
>> -                       s += sz - 1;
>> -                       if ((unsigned char)*s == c)
>> -                               return (char*)s;
>> -               }
>> -       }
>> -       return NULL;
>> -}
>> +       if (!s || !*s) return NULL;
>> +    while (*(s + 1)) s++;
>> +       return (*s == c) ? (char *) s : NULL;
>> +}
> 
> Code size for this particular fragment depends on what stupid things
> compiler feels compelled to do. My compiler does fewer of them
> when I use "*s == (char)c" instead.
> 
> Also, we can drop s == NULL case, none of the callers need that to work
> (similarly to all the usual string functions not expected to work on NULLs).

Hi,

please don't do this, this will bite us further up the road,
similarly as all string functions that save a check and break
havoc afterwards. In my opinion as a self taught spare time
and for fun coder functions should be able to handle whatever arg
you throw at them in a polite way, by removing the check
from the function it is only shifted to some other part 
of the caller code and potentially needs to be added every
time last_char_is is called (or at least this limitation
needs to be taken into account).  
In this specific case supporting returning NULL for a NULL
string doesn't modify the API as the function already
returns NULL for the 0-length string.

OTOH you are the boss and know better than me ;-)

Ciao,
Tito

> So...
> 
> char* FAST_FUNC last_char_is(const char *s, int c)
> {
>         if (!s[0])
>                 return NULL;
>         while (s[1])
>                 s++;
>         return (*s == (char)c) ? (char *) s : NULL;
> }
> 
>    0:   89 c1                   mov    %eax,%ecx
>    2:   80 38 00                cmpb   $0x0,(%eax)
>    5:   74 12                   je     19 <last_char_is+0x19>
>    7:   80 79 01 00             cmpb   $0x0,0x1(%ecx)
>    b:   74 03                   je     10 <last_char_is+0x10>
>    d:   41                      inc    %ecx
>    e:   eb f7                   jmp    7 <last_char_is+0x7>
>   10:   31 c0                   xor    %eax,%eax
>   12:   38 11                   cmp    %dl,(%ecx)
>   14:   75 05                   jne    1b <last_char_is+0x1b>
>   16:   89 c8                   mov    %ecx,%eax
>   18:   c3                      ret
>   19:   31 c0                   xor    %eax,%eax
>   1b:   c3                      ret
> 
> Almost perfect. If it'd also realize that copying EAX to ECX
> and back can be avoided...
> 


More information about the busybox mailing list