[PATCH] Removed the unsafe ngx_memcmp() wrapper for memcmp(3)

[Alejandro Colomar]

Hi Maxim!

On 11/8/22 10:50, Maxim Dounin wrote:
[...]

> 
> Another possibility is that these casts were simply added based on
> the ngx_str*() macro definitions and weren't actually needed.

Yeah, that was my most-likely guess.  But wanted to consider other possibilities 
just in case.

> Especially given that ngx_memcmp() was only used by ngx_memn2cmp()
> at that time, which is actually a string function.
> 
>>> Similar casts are used by almost all nginx string macro
>>> definitions, since most C library functions accept "char *", while
>>> nginx uses "u_char *" for strings, and this results in useless
>>> warnings.
>>
>> That's true of str...(3) functions from <string.h>, but the mem...(3) functions
>> from <string.h> use 'void *', so conversions are automatic, and casts are
>> unnecessary (after ANSI/ISO C89; previously, K&R C did use 'char *').
> 
> Sure.  The point is: similar casts are used by ngx_str*() macro
> definitions, so it might be copied from there even if not really
> needed.  And another point is: avoiding casts in ngx_memcmp()
> won't change anything, since there are lots of places with similar
> casts.

Yes, this one is (almost) a no-op change.  However, I'll still send the patch, 
since it's just a one-line patch, and more because of readability.  Not 
readability of the wrapper, which is as readable with or without the cast, but 
about programmers trying to understand why the cast is there and what can it be 
doing, since it actually does nothing.  I think that's an improvement, even if 
small in this case.

[...]

> Not really.  We certainly do support Windows XP, but nginx also
> happened to work on HP-UX, AIX, and even on QNX at some point
> (though there were warnings due to unsigned time_t, and likely
> associated bugs).  Some platforms are documented here:
> 
> http://nginx.org/en/#tested_os_and_platforms
> 
> And we certainly at least try to support anything reasonable /
> usable.
> 
> On the other hand, it might be good enough to require -Wno-error
> or an equivalent on ancient platforms.

Yeah, that makes sense.

[...]

> I tend to agree that we can drop casts from the ngx_memcmp()
> macro.
> 
> Note though that it's mostly nop change for the reasons given
> above.
> 

So, I'll send the patch in a moment.

Cheers,

Alex

-- 
<http://www.alejandro-colomar.es/>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: OpenPGP_signature
Type: application/pgp-signature
Size: 833 bytes
Desc: OpenPGP digital signature
URL: <http://mailman.nginx.org/pipermail/nginx-devel/attachments/20221108/e1e0c45b/attachment.bin>