[PATCH] assert.h: allow gcc to detect assert(a = 1) errors

[Jim Meyering]

On Wed, Dec 14, 2016 at 1:13 AM, Florian Weimer <fweimer@redhat.com> wrote:
> On 11/26/2016 07:14 AM, Jim Meyering wrote:
>
>> Here's a proposed ChangeLog entry:
>>
>> 2016-11-25  Jim Meyering  <meyering@fb.com>
>>
>>         Let gcc detect assert(a = 1) errors.
>>         * assert/assert.h (assert): Rewrite assert's definition so that a
>>         s/==/=/ typo, e.g., assert(errno = ENOENT) is not hidden from
>>         gcc's -Wparentheses by assert-added parentheses.  The new
>>         definition uses "if (expr) /* empty */; else __assert_fail...",
>>         so gcc -Wall will now detect that type of error in an assert, too.
>>         The __STRICT_ANSI__ disjunct is to avoid the warning that
>> -Wpedantic
>>         would otherwise issue for the use of ({...}).
>
> Isn't this warning suppressed for system headers with -Wpedantic?

Thanks for the review.

Good point. It's really for -ansi -pedantic, I suppose. Updated the log message.

>>         I would have preferred
>>         to use __extension__ to mark that, but doing so would mistakenly
>>         suppress warnings about any extension in the user-supplied "expr".
>>         E.g., "assert ( ({1;}) )" must continue to evoke a warning.
>>         https://bugzilla.redhat.com/1105335
>
> The following is a bit silly.  glibc adheres to the GNU standards, which
> require that the ChangeLog only describes the change as such, not its
> rationale.  So the entry should probably look like this.
>
>         Let GCC detect assert (a = 1) errors.
>         * assert/assert.h (assert): Introduce __STRICT_ANSI__ variant.
>         Use a GCC extension and an if statement for GNU compilers.

Thanks. I didn't realize glibc used such a strict interpretation.
Happy to adjust. How's this (also in the attached, rebased commit):

        Let gcc detect assert(a = 1) errors.
        * assert/assert.h (assert) Rewrite, retaining the old definintion
        when required, but otherwise putting the expression as-is in an "if"
        expression (hence, with no added parentheses) within a statement
        expression.
-------------- next part --------------
From 9cb1a7fd2ac0d7afc62e5a1e742c3819e1ac4346 Mon Sep 17 00:00:00 2001
From: Jim Meyering <meyering@fb.com>
Date: Thu, 5 Jun 2014 10:42:05 -0700
Subject: [PATCH] assert.h: allow gcc to detect assert(a = 1) errors

* assert/assert.h (assert): Rewrite assert's definition so that
a s/==/=/ typo, e.g., assert(errno = ENOENT) is not hidden from
gcc's -Wparentheses by assert-added parentheses.  The new definition
uses "if (expr) /* empty */; else __assert_fail...", so
gcc -Wall will now detect that type of error in an assert, too.
The __STRICT_ANSI__ disjunct is to make this work also with both
-ansi and  -pedantic, which would reject the use of ({...}).
I would have preferred to use __extension__ to mark that, but
doing so would mistakenly suppress warnings about any extension
in the user-supplied "expr".
E.g., "assert ( ({1;}) )" must continue to evoke a warning.
---
 ChangeLog       |  8 ++++++++
 assert/assert.h | 21 +++++++++++++++++----
 2 files changed, 25 insertions(+), 4 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index c08b711..0779419 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,11 @@
+2016-11-25  Jim Meyering  <meyering@fb.com>
+
+	Let gcc detect assert(a = 1) errors.
+	* assert/assert.h (assert) Rewrite, retaining the old definintion
+	when required, but otherwise putting the expression as-is in an "if"
+	expression (hence, with no added parentheses) within a statement
+	expression.
+
 2016-12-14  Joseph Myers  <joseph@codesourcery.com>

 	[BZ #20947]
diff --git a/assert/assert.h b/assert/assert.h
index 729edeb..0f25131 100644
--- a/assert/assert.h
+++ b/assert/assert.h
@@ -82,10 +82,23 @@ extern void __assert (const char *__assertion, const char *__file, int __line)

 __END_DECLS

-# define assert(expr)							\
-  ((expr)								\
-   ? __ASSERT_VOID_CAST (0)						\
-   : __assert_fail (#expr, __FILE__, __LINE__, __ASSERT_FUNCTION))
+/* When possible, define assert so that it does not add extra
+   parentheses around EXPR.  Otherwise, those added parentheses would
+   suppress warnings we'd expect to be detected by gcc's -Wparentheses.  */
+# if !defined __GNUC__ || defined __STRICT_ANSI__
+#  define assert(expr)							\
+    ((expr)								\
+     ? __ASSERT_VOID_CAST (0)						\
+     : __assert_fail (#expr, __FILE__, __LINE__, __ASSERT_FUNCTION))
+# else
+#  define assert(expr)							\
+    ({									\
+      if (expr)								\
+        ; /* empty */							\
+      else								\
+        __assert_fail (#expr, __FILE__, __LINE__, __ASSERT_FUNCTION);	\
+    })
+# endif

 # ifdef	__USE_GNU
 #  define assert_perror(errnum)						\
-- 
2.9.3