Re: [PATCH] libbb/last_char_is: rewrite for smaller and faster code
On Saturday, July 4, 2020, Harald van Dijk wrote: > > This is really the same problem as with standard library functions such as strchr(). The function should have a return type of char* if its argument has type char*. The function should have a return type of const char* if its argument has type const char*. The current signature is the only signature that allows all code that should be valid, at the expense of not rejecting code that should be invalid. Just to give a note: The const-correctness problem could have been avoided if the function API returns an offset (of `size_t` type) rather than the pointer directly. But it may be too late for the standard library to correct this historical mistake. ___ busybox mailing list busybox@busybox.net http://lists.busybox.net/mailman/listinfo/busybox
Re: [PATCH] libbb/last_char_is: rewrite for smaller and faster code
On 04/07/2020 09:56, Emmanuel Deloget wrote:
On Fri, Jul 3, 2020 at 9:15 PM Tito wrote:
On 7/3/20 6:13 PM, Xabier Oneca -- xOneca wrote:
Also, there's a warning compiling this function: return discards
‘const’ qualifier from pointer target type
Hi,
cast it?
char* FAST_FUNC last_char_is(const char *s, int c)
{
if (!s || !*s) return NULL;
while (*(++s));
return (*(--s) == c) ? (char *)s : NULL;
}
In favor of const-correctness, wouldn't it be better to return a const
char*? It's quite strange to promote a string to const and to a part
of it that is non-const (not to mention that if you feed the function
with a real const char* then the cast is not a good thing to do).
Existing callers rely on the return type being char*.
This is really the same problem as with standard library functions such
as strchr(). The function should have a return type of char* if its
argument has type char*. The function should have a return type of const
char* if its argument has type const char*. The current signature is the
only signature that allows all code that should be valid, at the expense
of not rejecting code that should be invalid.
busybox could define last_char_is as a macro that wraps this function,
which would optionally cast the return value to const char*. Such a
macro would be easy to define using _Generic, assuming the compiler is
recent enough to support it. This would ensure all code that should be
valid remains valid, while some (not all) code that should not be valid
becomes invalid. Such a macro could look like
#define last_char_is(s, c) \
(_Generic((s), \
const char *: (const char *) (last_char_is) ((s), (c)), \
default: (last_char_is) ((s), (c
But considering busybox uses standard library functions with the same
problem throughout the codebase, fixing it just for last_char_is while
leaving alone all other functions may not be worth the effort.
Cheers,
Harald van Dijk
___
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox
Re: [PATCH] libbb/last_char_is: rewrite for smaller and faster code
Hello,
On Fri, Jul 3, 2020 at 9:15 PM Tito wrote:
>
>
>
> On 7/3/20 6:13 PM, Xabier Oneca -- xOneca wrote:
> > Hi all,
> >
> >>> On Fri, 2020-07-03 at 08:12 +0200, Tito wrote:
> >>> [...]
> Improved version:
>
> char* last_char_is(const char *s, int c) {
> if (!s || !*s) return NULL;
> while (*(s + 1))s++;
> return (c == *s) ? (char *)s : NULL;
> }
> >
> > Let me "improve" the Tito's -11 bytes, probably introducing some subtle bug
> > :)
> >
> > char* FAST_FUNC last_char_is(const char *s, int c)
> > {
> > if (!s || !*s) return NULL;
> > while (*(++s));
> > return (*(--s) == c) ? s : NULL;
> > }
> >
> > function old new delta
> > last_char_is 58 42 -16
> > --
> > (add/remove: 0/0 grow/shrink: 0/1 up/down: 0/-16) Total: -16
> > bytes
> >textdata bss dec hex filename
> > 95644942351904 962588 eb01c busybox_old
> > 95643342351904 962572 eb00c busybox_unstripped
> >
> > Also, there's a warning compiling this function: return discards
> > ‘const’ qualifier from pointer target type
>
> Hi,
> cast it?
>
> char* FAST_FUNC last_char_is(const char *s, int c)
> {
> if (!s || !*s) return NULL;
> while (*(++s));
> return (*(--s) == c) ? (char *)s : NULL;
> }
>
In favor of const-correctness, wouldn't it be better to return a const
char*? It's quite strange to promote a string to const and to a part
of it that is non-const (not to mention that if you feed the function
with a real const char* then the cast is not a good thing to do).
>
> > Fun to play with you.
> >
> > Cheers,
> >
> > Xabier Oneca_,,_
Best regards,
-- Emmanuel Deloget
___
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox
