Message ID | 1405537923-28692-1-git-send-email-jim@meyering.net |
---|---|
State | New |
Headers | show |
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
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
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) \
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(-)