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

[Jim Meyering]

On Wed, Jul 16, 2014 at 1:43 PM, Jim Meyering <jim@meyering.net> wrote:
> On Wed, Jul 16, 2014 at 1:15 PM, Roland McGrath <roland@hack.frob.com> wrote:
>> This certainly should not go in during the current release freeze.
>> So you might not want to try to get too much discussion going until
>> the floodgates reopen.  Also, various key people might be less than
>> usually responsive until after Cauldron (and for me, another week of
>> vacation thereafter).
>
> Thanks for the quick feedback.
>
>> Isn't there an "empty statement" warning that might be on too?  We
>> certainly don't want the internals of the macro (the then clause in
>> your new version) to produce warnings of their own with any possible
>> combination of switches.
>
> -Wempty-body does not complain about the empty "then" clause
> inside that statement-expression.
>
>> You should be able to use __extension__ for GCC under -ansi.  But
>> perhaps that would wrongly allow use of extensions inside the user's
>> expression too.  If you're avoiding it for good reason, there should
>> be comments there explaining why.
>
> Good point. I confirmed that prefixing the ({... with __extension__
> lets me eliminate the __STRICT_ANSI__ disjunct, and that -ansi
> still warns about a use like "assert( ({1}) );".  I'll update the patch
> and repost here -- with a detailed test description --
> in a couple weeks.
>
>> I also wonder how modern GCCs running in integrated preprocessor
>> mode deal with the system header exception and/or __extension__ for
>> this sort of case (since in integrated preprocessor mode it can tell
>> which part came from the system header and which from the user).
>>
>> Any change like this is going to need a lot of detailed reporting
>> about testing with different compilers and option combinations and
>> sorts of expressions (i.e. ones using extensions or not and perhaps
>> various different sorts of extensions).
>
> I've tested it with -Wextra -Wall -W -O, with uses as statements and
> as expressions.  Also with and without -ansi.  I have not yet tested
> with a non-__GNUC__ compiler, but will do.

Hi Roland,
This slipped off my radar (for two years!).
I've attached an updated patch.

We *do* need that __STRICT_ANSI__ disjunct.
Otherwise, this would evoke no warning:

  $ gcc -isystem. -I. -Werror=pedantic k.c
  In file included from k.c:1:0:
  k.c: In function ‘main’:
  k.c:2:23: warning: ISO C forbids braced-groups within expressions [-Wpedantic]
   int main() { assert ( ({1;}) ); return 0; }

Tests I ran manually in a directory with the new assert.h file:

  Ensure that "assert(a=0)" now elicits a warning and this fails:
    printf '%s\n' '#include <assert.h>' \
      'int main() {int a = 1; assert (a = 0); return a;}' > k.c
    gcc -c -I. -Werror=parentheses k.c

  Ensure that clang does the same:
    clang -c -I. -Werror=parentheses k.c

  Ensure that this fails with a diagnostic about the stmt expression:
    printf '%s\n' '#include <assert.h>' \
      'int main() { assert (({1;})); return 0; }' > k.c
    gcc -c -isystem. -I. -Werror=pedantic k.c

  Ensure -Werror=pedantic does not complain about the stmt expression:
    printf '%s\n' '#include <assert.h>' \
      'int main() { assert (0); return 0; }' > k.c
    gcc -isystem. -I. -Werror=pedantic k.c

I didn't see any other tests for failing compilation.

Do you require a test suite addition for these? If so, would a single
bourne shell script be acceptable?
-------------- next part --------------
From 70387babebf4213baca2023adcf563df4aa7dbd0 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 avoid the warning that -Wpedantic
would otherwise issue for 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.
---
 assert/assert.h | 21 +++++++++++++++++----
 1 file changed, 17 insertions(+), 4 deletions(-)

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