Re: [PATCH 1/4] localename: -Wtautological-pointer-compare

[Bruno Haible]

Paul Eggert wrote:
> Problem found by xlclang 16.1 on AIX 7.2.
> * lib/localename.c (duplocale, freelocale):
> Omit unnecessary comparison of non-null args to NULL.

I disagree with this patch.

Compiler warnings are supposed to help us improve the code.

Replacing a function that starts with an entry check — which is a good
practice [1] — with one that operates in garbage-in - garbage-out fashion
is not an improvement; quite the opposite.

We already know how to handle this situation: see
  canonicalize-lgpl.c
  execl.c
  execle.c
  execlp.c
  execve.c
  execvpe.c
  getaddrinfo.c
  getdelim.c
  getpass.c
  glob.c
  random_r.c
  setenv.c
  tsearch.c
  unsetenv.c

[1] https://cwe.mitre.org/data/definitions/20.html


2023-01-13  Bruno Haible  <bruno@clisp.org>

        localename: Fix -Wtautological-pointer-compare warning in a better way.
        * lib/localename.c (duplocale, freelocale): Revert last patch.
        (_GL_ARG_NONNULL): Define to empty.

diff --git a/lib/localename.c b/lib/localename.c
index 5a178c68fe..8fe90e0bf2 100644
--- a/lib/localename.c
+++ b/lib/localename.c
@@ -18,6 +18,12 @@
 /* Native Windows code written by Tor Lillqvist <tml@iki.fi>.  */
 /* Mac OS X code written by Bruno Haible <bruno@clisp.org>.  */
 
+/* Don't use __attribute__ __nonnull__ in this compilation unit.  Otherwise gcc
+   optimizes away the locale == NULL tests below in duplocale() and 
freelocale(),
+   or xlclang reports -Wtautological-pointer-compare warnings for these tests.
+ */
+#define _GL_ARG_NONNULL(params)
+
 #include <config.h>
 
 /* Specification.  */
@@ -2967,6 +2973,10 @@ duplocale (locale_t locale)
   struct locale_hash_node *node;
   locale_t result;
 
+  if (locale == NULL)
+    /* Invalid argument.  */
+    abort ();
+
   node = (struct locale_hash_node *) malloc (sizeof (struct locale_hash_node));
   if (node == NULL)
     /* errno is set to ENOMEM.  */
@@ -3052,7 +3062,7 @@ void
 freelocale (locale_t locale)
 #undef freelocale
 {
-  if (locale == LC_GLOBAL_LOCALE)
+  if (locale == NULL || locale == LC_GLOBAL_LOCALE)
     /* Invalid argument.  */
     abort ();