diff mbox

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

Message ID CA+8g5KEJfwfhpePcMefKGBWzb=2auZ3WmA-RLFXRWOnzSiyoPg@mail.gmail.com
State New
Headers show

Commit Message

Jim Meyering Dec. 9, 2016, 3:17 a.m. UTC
On Fri, Nov 25, 2016 at 10:14 PM, Jim Meyering <jim@meyering.net> wrote:
> On Wed, Nov 23, 2016 at 11:36 PM, Florian Weimer <fweimer@redhat.com> wrote:
>> On 11/24/2016 03:21 AM, Jim Meyering wrote:
>>
>>> 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; }
>>
>>
>> Agreed.
>>
>>> Tests I ran manually in a directory with the new assert.h file:
>>
>>
>>> Do you require a test suite addition for these? If so, would a single
>>> bourne shell script be acceptable?
>>
>>
>> We currently lack the machinery for that.  It's not just that it would need
>> a shell script.  We also do not compile tests with headers as system
>> headers.
>>
>> The patch looks good to me, but it needs a ChangeLog entry.
>
> Thanks for the review.
> 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 ({...}).  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

Here's the complete, rebased patch. Ok to push, presuming I still have
commit access?
From 0954feae6411cc0de5f5cb6c7e007b972388139f 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.
---
 ChangeLog       | 15 +++++++++++++++
 assert/assert.h | 21 +++++++++++++++++----
 2 files changed, 32 insertions(+), 4 deletions(-)

Comments

Jim Meyering Dec. 14, 2016, 5:27 a.m. UTC | #1
On Thu, Dec 8, 2016 at 7:17 PM, Jim Meyering <jim@meyering.net> wrote:
> On Fri, Nov 25, 2016 at 10:14 PM, Jim Meyering <jim@meyering.net> wrote:
>> On Wed, Nov 23, 2016 at 11:36 PM, Florian Weimer <fweimer@redhat.com> wrote:
>>> On 11/24/2016 03:21 AM, Jim Meyering wrote:
>>>
>>>> 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; }
>>>
>>>
>>> Agreed.
>>>
>>>> Tests I ran manually in a directory with the new assert.h file:
>>>
>>>
>>>> Do you require a test suite addition for these? If so, would a single
>>>> bourne shell script be acceptable?
>>>
>>>
>>> We currently lack the machinery for that.  It's not just that it would need
>>> a shell script.  We also do not compile tests with headers as system
>>> headers.
>>>
>>> The patch looks good to me, but it needs a ChangeLog entry.
>>
>> Thanks for the review.
>> 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 ({...}).  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
>
> Here's the complete, rebased patch. Ok to push, presuming I still have
> commit access?

Friendly pre-holiday ping?
diff mbox

Patch

diff --git a/ChangeLog b/ChangeLog
index e19db5d..a0181e6 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,18 @@ 
+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 ({...}).  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
+
 2016-12-08  Joseph Myers  <joseph@codesourcery.com>

 	* Rules [$(run-built-tests) != no] (tests-expected): Add
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)						\