[PATCH] libbb/last_char_is: rewrite for smaller and faster code

Harald van Dijk harald at gigawatt.nl
Sat Jul 4 10:03:33 UTC 2020


On 04/07/2020 09:56, Emmanuel Deloget wrote:
> On Fri, Jul 3, 2020 at 9:15 PM Tito <farmatito at tiscali.it> 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


More information about the busybox mailing list