[PATCH v2] Removed the casts within ngx_memcmp()

[Maxim Dounin]

Hello!

On Tue, Nov 08, 2022 at 11:55:40AM +0100, Alejandro Colomar wrote:

> From: Alejandro Colomar <alx at kernel.org>
> 
> The casts are unnecessary, since memcmp(3)'s arguments are
> 'const void *', which allows implicit conversion from any pointer type.
> It might have been necessary in the times of K&R C, where 'void *'
> didn't exist yet, up until the early 2000s, because some old systems
> still had limited or no support for ISO C89.  Those systems passed away
> a long time ago, and current systems, even the oldest living ones, have
> support for ISO C89.
> 
> The cast in this case is (almost) innocuous, because it only hides
> warnings for conversions from integer to pointer such as:
> 
>     nxt_memcmp(n, "foo", 3);  // no warnings
>     memcmp(n, "foo", 3);      // warning: integer to pointer conversion
> 
> which is a difficult bug to write, since it's too obvious.  Such code
> will probably be caught in a code review, but there's always a small
> risk.  Since there's no reason to keep the small risk around, when we
> can just avoid it by removing the cast.
> 
> In general, it's better to avoid a cast if possible, since casts will
> disable many compiler warnings regarding type safety.
> 
> Apart from the small risk, there's a bigger concern about readability.
> Casts tend to disable compiler warnings, and so are unsafe by nature.
> A lot of care needs to be taken when casts are used, and a programmer
> reading the cast might wonder what was that specific cast trying to
> achieve.  The answer is clear: nothing.  So it's better to just prevent
> that moment of "wtf is this doing here".
> 
> Signed-off-by: Alejandro Colomar <alx at nginx.com>
> ---
>  src/core/ngx_string.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/src/core/ngx_string.h b/src/core/ngx_string.h
> index 0fb9be72..6c218e22 100644
> --- a/src/core/ngx_string.h
> +++ b/src/core/ngx_string.h
> @@ -145,7 +145,7 @@ ngx_copy(u_char *dst, u_char *src, size_t len)
>  
>  
>  /* msvc and icc7 compile memcmp() to the inline loop */
> -#define ngx_memcmp(s1, s2, n)  memcmp((const char *) s1, (const char *) s2, n)
> +#define ngx_memcmp(s1, s2, n)  memcmp(s1, s2, n)
>  
>  
>  u_char *ngx_cpystrn(u_char *dst, u_char *src, size_t n);

The suggested commit log seems to be referring to non-nginx code, 
and in general seems to be overcomplicated on the one hand, and 
lacks details supporting the claim that these casts are not needed 
in the nginx code as discussed in this thread on the other hand.

The suggested change, while being mostly style, does not take into 
account expected alignment of adjacent macro definitions: since 
the ngx_memcmp() macro definition is now short enough, it can be 
aligned similarly to ngx_memzero()/ngx_memcpy()/etc.

While here, fixed alignment of ngx_memmove() and ngx_movemem() 
macro definitions, these were off by 1 character.

# HG changeset patch
# User Maxim Dounin <mdounin at mdounin.ru>
# Date 1668004692 -10800
#      Wed Nov 09 17:38:12 2022 +0300
# Node ID fc79ea0724a92c1f463625a11ed4cb785cd342b7
# Parent  42bc158a47ecb3c2bd0396c723c307c757f2770e
Fixed alignment of ngx_memmove()/ngx_movemem() macro definitions.

diff --git a/src/core/ngx_string.h b/src/core/ngx_string.h
--- a/src/core/ngx_string.h
+++ b/src/core/ngx_string.h
@@ -140,8 +140,8 @@ ngx_copy(u_char *dst, u_char *src, size_
 #endif
 
 
-#define ngx_memmove(dst, src, n)   (void) memmove(dst, src, n)
-#define ngx_movemem(dst, src, n)   (((u_char *) memmove(dst, src, n)) + (n))
+#define ngx_memmove(dst, src, n)  (void) memmove(dst, src, n)
+#define ngx_movemem(dst, src, n)  (((u_char *) memmove(dst, src, n)) + (n))
 
 
 /* msvc and icc7 compile memcmp() to the inline loop */
# HG changeset patch
# User Maxim Dounin <mdounin at mdounin.ru>
# Date 1668005196 -10800
#      Wed Nov 09 17:46:36 2022 +0300
# Node ID 5269880f00df1e5ae08299165ec43435b759c5a3
# Parent  fc79ea0724a92c1f463625a11ed4cb785cd342b7
Removed casts from ngx_memcmp() macro.

Casts are believed to be not needed, since memcmp() has "const void *"
arguments since introduction of the "void" type in C89.  And on pre-C89
platforms nginx is unlikely to compile without warnings anyway, as there
are no casts in memcpy() and memmove() calls.

These casts were added in 1648:89a47f19b9ec without any details on why they
were added, and Igor does not remember details either.  The most plausible
explanation is that they were copied from ngx_strcmp() and were not really
needed even at that time.

Prodded by Alejandro Colomar.

diff --git a/src/core/ngx_string.h b/src/core/ngx_string.h
--- a/src/core/ngx_string.h
+++ b/src/core/ngx_string.h
@@ -145,7 +145,7 @@ ngx_copy(u_char *dst, u_char *src, size_
 
 
 /* msvc and icc7 compile memcmp() to the inline loop */
-#define ngx_memcmp(s1, s2, n)  memcmp((const char *) s1, (const char *) s2, n)
+#define ngx_memcmp(s1, s2, n)     memcmp(s1, s2, n)
 
 
 u_char *ngx_cpystrn(u_char *dst, u_char *src, size_t n);

-- 
Maxim Dounin
http://mdounin.ru/