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

login
register
mail settings
Submitter Jim Meyering
Date July 16, 2014, 7:12 p.m.
Message ID <1405537923-28692-1-git-send-email-jim@meyering.net>
Download mbox | patch
Permalink /patch/2096/
State New
Headers show

Comments

Jim Meyering - July 16, 2014, 7:12 p.m.
From: Jim Meyering <meyering@fb.com>

* 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 the parentheses added by assert.
The new definition uses "if (expr) ; else __assert_fail...", so
gcc -Wall will now detect that type of error in an assert, too.
---
 assert/assert.h | 21 +++++++++++++++++----
 1 file changed, 17 insertions(+), 4 deletions(-)
Roland McGrath - July 16, 2014, 8:15 p.m.
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).

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.

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.

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).


Thanks,
Roland
Jim Meyering - July 16, 2014, 8:43 p.m.
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.

Thanks,
Jim

Patch

diff --git a/assert/assert.h b/assert/assert.h
index 2661391..ffcf60c 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 (__STRING(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 (__STRING(expr), __FILE__, __LINE__, __ASSERT_FUNCTION))
+# else
+#  define assert(expr)							\
+    ({									\
+      if (expr)								\
+        ; /* empty */							\
+      else								\
+        __assert_fail (__STRING(expr), __FILE__, __LINE__, __ASSERT_FUNCTION);\
+    })
+# endif

 # ifdef	__USE_GNU
 #  define assert_perror(errnum)						\