Re: [PATCH] shrink last_char_is function even more

[Tito]

Il 02/09/20 14:23, dietmar.schind...@manrolandgoss.com ha scritto:
>> -Original Message-
>> From: busybox  On Behalf Of Tito
>> Sent: Monday, July 20, 2020 1:58 PM
>> To: busybox@busybox.net
>>
>> …
>> I for myself decided for being defensive and this until today
>> payed off for my little personal codebase.
> 
> How have you been able to determine that this "payed off"?

Hi,
because I know that I will make mistakes in the logic and I know
that I will have to fix the few programs that I wrote in  C +  GTK + Mysql
for my business for Linux and Windows 7/10 after years (despite comments in the 
source)
and I know it will be a terrible mess(e.g just because a third party data 
supplier
decided to change something to beautify it...)
but I also know that my string functions will not crash on NULL strings,
my pointers are always initialized to NULL , are always reset to NULL after 
free,
my mysql functions check for errors and escape input and that they always are 
so kind to tell
me if something is wrong (in the GUI and/or terminal) without the need to 
recompile with DEBUG statements,
thusly in the end I save a lot of time by being defensive rather than
sitting in front of a crashed program and scratching my head.
Time is my scarcest resource and is valuable so in this sense it "payed off". 

Ciao,
Tito

>>>  Checking for NULL "just in case" is defensive programming, which is
>>> very bad. It means the programmer does not know exactly what the
>>> function contracts are: it would be better named "sloppy programming".
>>> Please don't do this.
>>>
>>> --
>>>  Laurent
> 
> --
> Regards,
> Dietmar Schindler
> 
> manroland Goss web systems GmbH | Managing Director: Franz Kriechbaum
> Registered Office: Augsburg | Trade Register: AG Augsburg | HRB-No.: 32609 | 
> VAT: DE815764857
___
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox

RE: [PATCH] shrink last_char_is function even more

[dietmar.schindler]

> -Original Message-
> From: busybox  On Behalf Of Tito
> Sent: Monday, July 20, 2020 1:58 PM
> To: busybox@busybox.net
>
> …
> I for myself decided for being defensive and this until today
> payed off for my little personal codebase.

How have you been able to determine that this "payed off"?

> >  Checking for NULL "just in case" is defensive programming, which is
> > very bad. It means the programmer does not know exactly what the
> > function contracts are: it would be better named "sloppy programming".
> > Please don't do this.
> >
> > --
> >  Laurent

--
Regards,
Dietmar Schindler

manroland Goss web systems GmbH | Managing Director: Franz Kriechbaum
Registered Office: Augsburg | Trade Register: AG Augsburg | HRB-No.: 32609 | 
VAT: DE815764857

Confidentiality note:
This message and any attached documents may contain confidential or proprietary 
information of manroland|Goss. These materials are intended only for the use of 
the intended recipient. If you are not the intended recipient of this 
transmission, you are hereby notified that any distribution, disclosure, 
printing, copying, storage, modification or the taking of any action in 
reliance upon this transmission is strictly prohibited. Delivery of this 
message to any person other than the intended recipient shall not compromise or 
waive such confidentiality, privilege or exemption from disclosure as to this 
communication. If you have received this communication in error, please 
immediately notify the sender and delete the message from your system. All 
liability for viruses is excluded to the fullest extent permitted by law.

___
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox

Re: [PATCH] shrink last_char_is function even more

[Bernd Petrovitsch]

On 20/07/2020 05:46, Lauri Kasanen wrote:
> On Sun, 19 Jul 2020 22:31:09 +0200
> Tito  wrote:
> 
>> 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.
> 
> The param should be marked with the nonnull attribute, just like the
> libc string functions. Then the compiler will warn you if you try to
> pass NULL (may need higher optimization, warning levels, or the
> analyzer mode in complex cases).

And it wants an assert()/assert()-like statement at the 1st line
of the function so that one can actually check the nonnull-ness
in test/debug/regression-test/... situations.

MfG,
Bernd
-- 
There is no cloud, just other people computers.
-- https://static.fsf.org/nosvn/stickers/thereisnocloud.svg


pEpkey.asc
Description: application/pgp-keys
___
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox

Re: [PATCH] shrink last_char_is function even more

[Didier Kryn]

Le 20/07/2020 à 09:22, Laurent Bercot a écrit :
>  When writing and using a function that takes pointers, a C programmer
> should always be very aware of the kind of pointer the function expects.
> It is a programming error to pass NULL to a function expecting a pointer
> that cannot be NULL, and that error should be caught as early as
> possible. The nonnull attribute helps detect it at compile time. And
> at run time, if the function gets NULL, it should crash, as loudly as
> possible, in order for the bug to be fixed.
>
>  Checking for NULL "just in case" is defensive programming, which is
> very bad. It means the programmer does not know exactly what the
> function contracts are: it would be better named "sloppy programming".
> Please don't do this.

    There are two moments the error can be caught: at compile time, if
the compiler can determine wether the pointer is null or not, or at run
time if the previous is impossible. In the second case, the compiler
should insert the defensive code by itself into the caller. It is always
safer and more readable to establish this kind of contract and let the
compiler take care of the defense.

    This kind of contract exists in other languages but I can't remember
it in C. Is a non-null pointer a novelty of the C language or a gcc
extension?

    Didier


___
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox

Re: [PATCH] shrink last_char_is function even more

[Kang-Che Sung]

On Monday, July 20, 2020, Tito  wrote:
>
>
> On 7/20/20 9:22 AM, Laurent Bercot wrote:
>>> The param should be marked with the nonnull attribute, just like the
>>> libc string functions. Then the compiler will warn you if you try to
>>> pass NULL (may need higher optimization, warning levels, or the
>>> analyzer mode in complex cases).
>>
>>  Indeed.
>>
>>  A function that takes a pointer that *cannot* be NULL, and a function
>> that takes a pointer that *may* be NULL, are not the same thing at all.
>>  This is one of the main reasons while a lot of people find C pointers
>> difficult: a pointer can be used for two very different things, namely
>> unconditionally representing an object (passing it by address), or
>> representing the *possibility* of an object. In ocaml, the former would
>> would be typed "'a ref", and the latter "'a ref option", and those are
>> *not the same type*.
>>
>>  When writing and using a function that takes pointers, a C programmer
>> should always be very aware of the kind of pointer the function expects.
>> It is a programming error to pass NULL to a function expecting a pointer
>> that cannot be NULL, and that error should be caught as early as
>> possible. The nonnull attribute helps detect it at compile time. And
>> at run time, if the function gets NULL, it should crash, as loudly as
>> possible, in order for the bug to be fixed.
>
> Hi,
>
> while I agree with you in theory, the reality looks different to
> me, being an external observer, because with increasing code base,
> increasing complexity, increasing number of people messing with
> the code, increasing time pressure the result is a flood of CVE's
> and updates and patch Tuesdays, etc..
> So a question arises to me, aren't all this programmers able to do their
job or
> is this the result of sloppy programming or a deficiency of defensive
> programming taking into account that humans for all the aforementioned
> reasons will certainly do mistakes in a unpredictable but very effective
way?
> I for myself decided for being defensive and this until today
> payed off for my little personal codebase.
> I save you from showing examples of my defensive coding
> that I'm sure would horrify most of the list members ;-)
>

Hi. Just my small opinion here.

Coding defensively is not necessarily a wrong thing by itself. And I do
write things defensively for my programming job.

The key point is that coding defensively does not mean adding unnecessary,
sanity checks for everything. That would hurt performance. When a function
expects a non-null pointer argument from the caller (and you are sure that
caller code is sane already), use GCC's nonnull attribute and assertions.
That's what assert statements are for - conveying your code assumptions and
do the sanity checks for debugging and only in the debug build. That would
keep your "defensive" attitude without sacrificing any performance thing.
___
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox

Re: [PATCH] shrink last_char_is function even more

[Tito]

On 7/20/20 9:22 AM, Laurent Bercot wrote:
>> The param should be marked with the nonnull attribute, just like the
>> libc string functions. Then the compiler will warn you if you try to
>> pass NULL (may need higher optimization, warning levels, or the
>> analyzer mode in complex cases).
> 
>  Indeed.
> 
>  A function that takes a pointer that *cannot* be NULL, and a function
> that takes a pointer that *may* be NULL, are not the same thing at all.
>  This is one of the main reasons while a lot of people find C pointers
> difficult: a pointer can be used for two very different things, namely
> unconditionally representing an object (passing it by address), or
> representing the *possibility* of an object. In ocaml, the former would
> would be typed "'a ref", and the latter "'a ref option", and those are
> *not the same type*.
> 
>  When writing and using a function that takes pointers, a C programmer
> should always be very aware of the kind of pointer the function expects.
> It is a programming error to pass NULL to a function expecting a pointer
> that cannot be NULL, and that error should be caught as early as
> possible. The nonnull attribute helps detect it at compile time. And
> at run time, if the function gets NULL, it should crash, as loudly as
> possible, in order for the bug to be fixed.

Hi,

while I agree with you in theory, the reality looks different to 
me, being an external observer, because with increasing code base,
increasing complexity, increasing number of people messing with
the code, increasing time pressure the result is a flood of CVE's
and updates and patch Tuesdays, etc..
So a question arises to me, aren't all this programmers able to do their job or
is this the result of sloppy programming or a deficiency of defensive
programming taking into account that humans for all the aforementioned
reasons will certainly do mistakes in a unpredictable but very effective way?
I for myself decided for being defensive and this until today
payed off for my little personal codebase.
I save you from showing examples of my defensive coding
that I'm sure would horrify most of the list members ;-)

Ciao,
Tito

>  Checking for NULL "just in case" is defensive programming, which is
> very bad. It means the programmer does not know exactly what the
> function contracts are: it would be better named "sloppy programming".
> Please don't do this.
> 
> -- 
>  Laurent
___
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox

Re: [PATCH] shrink last_char_is function even more

[Laurent Bercot]

The param should be marked with the nonnull attribute, just like the
libc string functions. Then the compiler will warn you if you try to
pass NULL (may need higher optimization, warning levels, or the
analyzer mode in complex cases).


 Indeed.

 A function that takes a pointer that *cannot* be NULL, and a function
that takes a pointer that *may* be NULL, are not the same thing at all.
 This is one of the main reasons while a lot of people find C pointers
difficult: a pointer can be used for two very different things, namely
unconditionally representing an object (passing it by address), or
representing the *possibility* of an object. In ocaml, the former would
would be typed "'a ref", and the latter "'a ref option", and those are
*not the same type*.

 When writing and using a function that takes pointers, a C programmer
should always be very aware of the kind of pointer the function expects.
It is a programming error to pass NULL to a function expecting a pointer
that cannot be NULL, and that error should be caught as early as
possible. The nonnull attribute helps detect it at compile time. And
at run time, if the function gets NULL, it should crash, as loudly as
possible, in order for the bug to be fixed.

 Checking for NULL "just in case" is defensive programming, which is
very bad. It means the programmer does not know exactly what the
function contracts are: it would be better named "sloppy programming".
Please don't do this.

--
 Laurent

___
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox

Re: [PATCH] shrink last_char_is function even more

[Lauri Kasanen]

On Sun, 19 Jul 2020 22:31:09 +0200
Tito  wrote:

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

The param should be marked with the nonnull attribute, just like the
libc string functions. Then the compiler will warn you if you try to
pass NULL (may need higher optimization, warning levels, or the
analyzer mode in complex cases).

- Lauri
___
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox

Re: [PATCH] shrink last_char_is function even more

[Michael Conrad]

On 7/19/2020 2:48 PM, Denys Vlasenko wrote:

strrchr(s,c) will first find the end of s, then go backwards looking for c.
The second part is wasted work, we only need to check the*last*  char == c.


Maybe a naive implementation, but why wouldn't they just record the last 
occurrence on a forward scan?


___
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox

Re: [PATCH] shrink last_char_is function even more

[Tito]

On 7/19/20 8:48 PM, Denys Vlasenko wrote:
> On Tue, Jul 7, 2020 at 9:17 PM Tito  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
>>textdata bss dec hex filename
>>  981219   169071872  98   f423e busybox_old
>>  981208   169071872  87   f4233 busybox_unstripped
>>
>> Patch for review:
>>
>> --- libbb/last_char_is.c.orig   2020-07-05 09:54:24.737931000 +0200
>> +++ libbb/last_char_is.c2020-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 00cmpb   $0x0,(%eax)
>5:   74 12   je 19 
>7:   80 79 01 00 cmpb   $0x0,0x1(%ecx)
>b:   74 03   je 10 
>d:   41  inc%ecx
>e:   eb f7   jmp7 
>   10:   31 c0   xor%eax,%eax
>   12:   38 11   cmp%dl,(%ecx)
>   14:   75 05   jne1b 
>   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...
> 
___
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox

Re: [PATCH] shrink last_char_is function even more

[Denys Vlasenko]

On Tue, Jul 7, 2020 at 9:17 PM Tito  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
>textdata bss dec hex filename
>  981219   169071872  98   f423e busybox_old
>  981208   169071872  87   f4233 busybox_unstripped
>
> Patch for review:
>
> --- libbb/last_char_is.c.orig   2020-07-05 09:54:24.737931000 +0200
> +++ libbb/last_char_is.c2020-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).

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 00cmpb   $0x0,(%eax)
   5:   74 12   je 19 
   7:   80 79 01 00 cmpb   $0x0,0x1(%ecx)
   b:   74 03   je 10 
   d:   41  inc%ecx
   e:   eb f7   jmp7 
  10:   31 c0   xor%eax,%eax
  12:   38 11   cmp%dl,(%ecx)
  14:   75 05   jne1b 
  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...
___
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox

Re: [PATCH] shrink last_char_is function even more

[Denys Vlasenko]

On Thu, Jul 16, 2020 at 7:53 AM Stefan Seyfried
 wrote:
>
> Am 09.07.20 um 21:13 schrieb Tito:
> > On 7/9/20 9:56 PM, Martin Lewis wrote:
> >> Please note that my original patch is still smaller:
> >> http://lists.busybox.net/pipermail/busybox/2020-June/088026.html
> >
> > Hi,
> > yes I know. This is smaller than what is in git now.
> > I understood that Denis rejected your patch:
> > "This scans the string twice, unnecessarily. Let's not do that."
>
> I'd really like an explanation, because for me this sentence looks
> wrong, I cannot see two scans, only one strrchr(). Or is strrchr so wrong?

strrchr(s,c) will first find the end of s, then go backwards looking for c.
The second part is wasted work, we only need to check the *last* char == c.
___
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox

Re: [PATCH] shrink last_char_is function even more

[Stefan Seyfried]

Am 09.07.20 um 21:13 schrieb Tito:
> On 7/9/20 9:56 PM, Martin Lewis wrote:
>> Please note that my original patch is still smaller:
>> http://lists.busybox.net/pipermail/busybox/2020-June/088026.html
> 
> Hi,
> yes I know. This is smaller than what is in git now.
> I understood that Denis rejected your patch:
> "This scans the string twice, unnecessarily. Let's not do that."

I'd really like an explanation, because for me this sentence looks
wrong, I cannot see two scans, only one strrchr(). Or is strrchr so wrong?

Your speed test already showed that Martins version is faster, maybe
because of highly asm-optimized str* functions in glibc? Would be
interesting to benchmark on some old slow processor type ;-)
-- 
Stefan Seyfried

"For a successful technology, reality must take precedence over
 public relations, for nature cannot be fooled." -- Richard Feynman
___
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox

Re: [PATCH] shrink last_char_is function even more

[Tito]

On 7/9/20 9:13 PM, Tito wrote:
> On 7/9/20 9:56 PM, Martin Lewis wrote:
>> Please note that my original patch is still smaller:
>> http://lists.busybox.net/pipermail/busybox/2020-June/088026.html
> 
> Hi,
> yes I know. This is smaller than what is in git now.
> I understood that Denis rejected your patch:
> "This scans the string twice, unnecessarily. Let's not do that."
> Can't say If he will like mine.
>  
>> I'm not sure whether it's faster, it would be interesting to compare them.
> 
> Can you suggest how could this be achieved? a test program?

Hi,
by some rough test i did your version is way faster:

real0m0.005s
user0m0.006s
sys 0m0.002s

vs

real0m0.095s
user0m0.102s
sys 0m0.001s

This is on 1 iterations on a 3000 char array.

Ciao,
Tito
 
> Ciao,
> Tito
> 
>> On Tue, 7 Jul 2020 at 14:17, Tito > > 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  98   f423e busybox_old
>>  981208   16907    1872  87   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;
>> +}
>>
>>
>> The alternative version:
>>
>> char* FAST_FUNC last_char_is(const char *s, int c)
>> {
>>       if (!s || !*s) return NULL;
>>       while (*(++s));
>>       return (*(--s) == c) ? (char *)s : NULL;
>> }
>>
>> that was also posted to the list on my system was bigger:
>>
>> function                                             old     new   delta
>> last_char_is                                          53      46      -7
>> 
>> --
>> (add/remove: 0/0 grow/shrink: 0/1 up/down: 0/-7)               Total: -7 
>> bytes
>>
>> Ciao,
>> Tito
>>
>>
>> P.S.: test program, if you are aware of other corner cases please tell 
>> me:
>> 
>> 
>>
>> char* last_char_is(const char *s, int c)
>> {
>>         if (!s || !*s)
>>                 return NULL;
>>     while (*(s + 1))
>>                 s++;
>>         return (*s == c) ? (char *) s : NULL;
>> }
>>
>> int main (int argc, char **argv) {
>>         char *c;
>>         int ret = 0;
>>
>>         printf("Test 1 'prova','a' : ");
>>         c = last_char_is("prova", 'a');
>>         if (c !=  NULL && *c == 'a') {
>>                 puts("PASSED");
>>         } else {
>>                 puts("FAILED");
>>                 ret |= 1;
>>         }
>>         printf("Test 2 ''     ,'x' : ");
>>         c = last_char_is("", 'x');
>>         if (c != NULL) {
>>                 puts("FAILED");
>>                 ret |= 1;
>>
>>         } else {
>>                 puts("PASSED");
>>         }
>>         printf("Test 3  NULL  ,'3' : ");
>>         c = last_char_is(NULL, 'e');
>>         if (c != NULL) {
>>                 puts("FAILED");
>>                 ret |= 1;
>>         } else {
>>                 puts("PASSED");
>>         }
>>         printf("Test 4 'prova','x' : ");
>>         c = last_char_is("prova", 'x');
>>         if (c != NULL) {
>>                 puts("FAILED");
>>                 ret |= 1;
>>         } else {
>>                 puts("PASSED");
>>         }
>>         printf("Test 5 'prova','\\n': ");
>>         c = last_char_is("prova", '\n');
>>         if (c != NULL) {
>>                 puts("FAILED");
>>                 ret |= 1;
>>         } 

Re: [PATCH] shrink last_char_is function even more

[Tito]

On 7/9/20 9:56 PM, Martin Lewis wrote:
> Please note that my original patch is still smaller:
> http://lists.busybox.net/pipermail/busybox/2020-June/088026.html

Hi,
yes I know. This is smaller than what is in git now.
I understood that Denis rejected your patch:
"This scans the string twice, unnecessarily. Let's not do that."
Can't say If he will like mine.
 
> I'm not sure whether it's faster, it would be interesting to compare them.

Can you suggest how could this be achieved? a test program?

Ciao,
Tito

> On Tue, 7 Jul 2020 at 14:17, Tito  > 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  98   f423e busybox_old
>  981208   16907    1872  87   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;
> +}
> 
> 
> The alternative version:
> 
> char* FAST_FUNC last_char_is(const char *s, int c)
> {
>       if (!s || !*s) return NULL;
>       while (*(++s));
>       return (*(--s) == c) ? (char *)s : NULL;
> }
> 
> that was also posted to the list on my system was bigger:
> 
> function                                             old     new   delta
> last_char_is                                          53      46      -7
> 
> --
> (add/remove: 0/0 grow/shrink: 0/1 up/down: 0/-7)               Total: -7 
> bytes
> 
> Ciao,
> Tito
> 
> 
> P.S.: test program, if you are aware of other corner cases please tell me:
> 
> 
> 
> char* last_char_is(const char *s, int c)
> {
>         if (!s || !*s)
>                 return NULL;
>     while (*(s + 1))
>                 s++;
>         return (*s == c) ? (char *) s : NULL;
> }
> 
> int main (int argc, char **argv) {
>         char *c;
>         int ret = 0;
> 
>         printf("Test 1 'prova','a' : ");
>         c = last_char_is("prova", 'a');
>         if (c !=  NULL && *c == 'a') {
>                 puts("PASSED");
>         } else {
>                 puts("FAILED");
>                 ret |= 1;
>         }
>         printf("Test 2 ''     ,'x' : ");
>         c = last_char_is("", 'x');
>         if (c != NULL) {
>                 puts("FAILED");
>                 ret |= 1;
> 
>         } else {
>                 puts("PASSED");
>         }
>         printf("Test 3  NULL  ,'3' : ");
>         c = last_char_is(NULL, 'e');
>         if (c != NULL) {
>                 puts("FAILED");
>                 ret |= 1;
>         } else {
>                 puts("PASSED");
>         }
>         printf("Test 4 'prova','x' : ");
>         c = last_char_is("prova", 'x');
>         if (c != NULL) {
>                 puts("FAILED");
>                 ret |= 1;
>         } else {
>                 puts("PASSED");
>         }
>         printf("Test 5 'prova','\\n': ");
>         c = last_char_is("prova", '\n');
>         if (c != NULL) {
>                 puts("FAILED");
>                 ret |= 1;
>         } else {
>                 puts("PASSED");
>         }
>         printf("Test 6 'prova','\\0': ");
>         c = last_char_is("prova", 0);
>         if (c != NULL) {
>                 puts("FAILED");
>                 ret |= 1;
>         } else {
>                 puts("PASSED");
>         }
>         return ret;
> }
> 
> 

Re: [PATCH] shrink last_char_is function even more

[Martin Lewis]

Please note that my original patch is still smaller:
http://lists.busybox.net/pipermail/busybox/2020-June/088026.html

I'm not sure whether it's faster, it would be interesting to compare them.

On Tue, 7 Jul 2020 at 14:17, Tito  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
>textdata bss dec hex filename
>  981219   169071872  98   f423e busybox_old
>  981208   169071872  87   f4233 busybox_unstripped
>
> Patch for review:
>
> --- libbb/last_char_is.c.orig   2020-07-05 09:54:24.737931000 +0200
> +++ libbb/last_char_is.c2020-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;
> +}
>
>
> The alternative version:
>
> char* FAST_FUNC last_char_is(const char *s, int c)
> {
>   if (!s || !*s) return NULL;
>   while (*(++s));
>   return (*(--s) == c) ? (char *)s : NULL;
> }
>
> that was also posted to the list on my system was bigger:
>
> function old new   delta
> last_char_is  53  46  -7
>
> --
> (add/remove: 0/0 grow/shrink: 0/1 up/down: 0/-7)   Total: -7
> bytes
>
> Ciao,
> Tito
>
>
> P.S.: test program, if you are aware of other corner cases please tell me:
>
> 
>
> char* last_char_is(const char *s, int c)
> {
> if (!s || !*s)
> return NULL;
> while (*(s + 1))
> s++;
> return (*s == c) ? (char *) s : NULL;
> }
>
> int main (int argc, char **argv) {
> char *c;
> int ret = 0;
>
> printf("Test 1 'prova','a' : ");
> c = last_char_is("prova", 'a');
> if (c !=  NULL && *c == 'a') {
> puts("PASSED");
> } else {
> puts("FAILED");
> ret |= 1;
> }
> printf("Test 2 '' ,'x' : ");
> c = last_char_is("", 'x');
> if (c != NULL) {
> puts("FAILED");
> ret |= 1;
>
> } else {
> puts("PASSED");
> }
> printf("Test 3  NULL  ,'3' : ");
> c = last_char_is(NULL, 'e');
> if (c != NULL) {
> puts("FAILED");
> ret |= 1;
> } else {
> puts("PASSED");
> }
> printf("Test 4 'prova','x' : ");
> c = last_char_is("prova", 'x');
> if (c != NULL) {
> puts("FAILED");
> ret |= 1;
> } else {
> puts("PASSED");
> }
> printf("Test 5 'prova','\\n': ");
> c = last_char_is("prova", '\n');
> if (c != NULL) {
> puts("FAILED");
> ret |= 1;
> } else {
> puts("PASSED");
> }
> printf("Test 6 'prova','\\0': ");
> c = last_char_is("prova", 0);
> if (c != NULL) {
> puts("FAILED");
> ret |= 1;
> } else {
> puts("PASSED");
> }
> return ret;
> }
>
> 
> ___
> busybox mailing list
> busybox@busybox.net
> http://lists.busybox.net/mailman/listinfo/busybox
>
___
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox