[PATCH 4/4] last_char_is: code shrink

[Jody Bruchon]

You're obviously right. I didn't look at it as a diff. Sorry.

I would like to offer this (somewhat tested) code instead:


char* FAST_FUNC last_char_is(char *s, char c)
{
     if (s && *s) {
         while (*s != '\0') s++;
         s--;
         if (*s == c) return s;
     }
     return NULL;
}


For gcc -O2 (strlen="old", strrchr version blank, above="new"):
    text    data     bss     dec     hex filename
      91       0       0      91      5b last_char_is.o
      97       0       0      97      61 last_char_is_new.o
     139       0       0     139      8b last_char_is_old.o

for gcc -Os:
    text    data     bss     dec     hex filename
      78       0       0      78      4e last_char_is.o
      85       0       0      85      55 last_char_is_new.o
     105       0       0     105      69 last_char_is_old.o

The above code should be more efficient than both the strlen and strrchr 
versions while still being smaller than the current strlen version. It 
only scans the string once, doesn't do any unnecessary work, and rejects 
both null and empty string inputs. I am not certain of the consequences 
of dropping 'const' from the function, but it looks like the function 
would be making a new copy of a pointer variable to work with anyway.

I wrote a version that calls strlen() but it was larger. All my tests 
were done on x86_64.

On 2020-07-01 09:34, Martin Lewis wrote:
> Notice that there's a "-" before the line with strlen.
> Here's the final implementation to simplify reading:
>
> char* FAST_FUNC last_char_is(const char *s, int c)
> {
>     if (s) {
>         char *index = strrchr(s, c);
>         if (index && *(index + 1) == '\0')
>             return index;
>     }
>     return NULL;
> }
>
> On Tue, 30 Jun 2020 at 10:59, Jody Bruchon <jody at jodybruchon.com 
> <mailto:jody at jodybruchon.com>> wrote:
>
>     strlen scans the string; strrchr also scans the string.
>
>     On 2020-07-01 10:23, Martin Lewis wrote:
>     > Hi,
>     >
>     > I'm not sure what I'm missing, from a quick glance at strrchr
>     > (glibc's) it seems that it scans the string only once?
>     >
>     > Thank you,
>     > Martin
>     >
>     > On Tue, 30 Jun 2020 at 01:34, Denys Vlasenko
>     <vda.linux at googlemail.com <mailto:vda.linux at googlemail.com>
>     > <mailto:vda.linux at googlemail.com
>     <mailto:vda.linux at googlemail.com>>> wrote:
>     >
>     >     This scans the string twice, unnecessarily. Let's not do that.
>     >
>     >     On Thu, Jun 11, 2020 at 3:45 PM Martin Lewis
>     >     <martin.lewis.x84 at gmail.com
>     <mailto:martin.lewis.x84 at gmail.com>
>     <mailto:martin.lewis.x84 at gmail.com
>     <mailto:martin.lewis.x84 at gmail.com>>>
>     >     wrote:
>     >     >
>     >     > function      old    new
>     >      delta
>     >     > last_char_is       53     30
>     >        -23
>     >     >
>     >
>      ------------------------------------------------------------------------------
>     >     > (add/remove: 0/0 grow/shrink: 0/1 up/down: 0/-23)      Total:
>     >     -23 bytes
>     >     >    text    data     bss     dec     hex filename
>     >     >  981322   16915    1872 1000109   f42ad busybox_old
>     >     >  981299   16915    1872 1000086   f4296 busybox_unstripped
>     >     >
>     >     > Signed-off-by: Martin Lewis <martin.lewis.x84 at gmail.com
>     <mailto:martin.lewis.x84 at gmail.com>
>     >     <mailto:martin.lewis.x84 at gmail.com
>     <mailto:martin.lewis.x84 at gmail.com>>>
>     >     > ---
>     >     >  libbb/last_char_is.c | 9 ++++-----
>     >     >  1 file changed, 4 insertions(+), 5 deletions(-)
>     >     >
>     >     > diff --git a/libbb/last_char_is.c b/libbb/last_char_is.c
>     >     > index 66f2e3635..1fff08f9f 100644
>     >     > --- a/libbb/last_char_is.c
>     >     > +++ b/libbb/last_char_is.c
>     >     > @@ -13,11 +13,10 @@
>     >     >   */
>     >     >  char* FAST_FUNC last_char_is(const char *s, int c)
>     >     >  {
>     >     > -       if (s && *s) {
>     >     > -               size_t sz = strlen(s) - 1;
>     >     > -               s += sz;
>     >     > -               if ( (unsigned char)*s == c)
>     >     > -                       return (char*)s;
>     >     > +       if (s) {
>     >     > +               char *index = strrchr(s, c);
>     >     > +               if (index && *(index + 1) == '\0')
>     >     > +                       return index;
>     >     >         }
>     >     >         return NULL;
>     >     >  }
>     >     > --
>     >     > 2.11.0
>     >     >
>     >     > _______________________________________________
>     >     > busybox mailing list
>     >     > busybox at busybox.net <mailto:busybox at busybox.net>
>     <mailto:busybox at busybox.net <mailto:busybox at busybox.net>>
>     >     > http://lists.busybox.net/mailman/listinfo/busybox
>     >
>     >
>     > _______________________________________________
>     > busybox mailing list
>     > busybox at busybox.net <mailto:busybox at busybox.net>
>     > http://lists.busybox.net/mailman/listinfo/busybox
>
>     _______________________________________________
>     busybox mailing list
>     busybox at busybox.net <mailto:busybox at busybox.net>
>     http://lists.busybox.net/mailman/listinfo/busybox
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.busybox.net/pipermail/busybox/attachments/20200701/a32075c8/attachment.html>