* [PATCH] assert.h: allow gcc to detect assert(a = 1) errors @ 2014-07-16 19:12 Jim Meyering 2014-07-16 20:15 ` Roland McGrath 0 siblings, 1 reply; 17+ messages in thread From: Jim Meyering @ 2014-07-16 19:12 UTC (permalink / raw) To: libc-alpha; +Cc: Roland McGrath 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(-) 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) \ -- 2.0.1 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] assert.h: allow gcc to detect assert(a = 1) errors 2014-07-16 19:12 [PATCH] assert.h: allow gcc to detect assert(a = 1) errors Jim Meyering @ 2014-07-16 20:15 ` Roland McGrath 2014-07-16 20:44 ` Jim Meyering 0 siblings, 1 reply; 17+ messages in thread From: Roland McGrath @ 2014-07-16 20:15 UTC (permalink / raw) To: Jim Meyering; +Cc: libc-alpha 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] assert.h: allow gcc to detect assert(a = 1) errors 2014-07-16 20:15 ` Roland McGrath @ 2014-07-16 20:44 ` Jim Meyering 2016-11-24 2:22 ` Jim Meyering 0 siblings, 1 reply; 17+ messages in thread From: Jim Meyering @ 2014-07-16 20:44 UTC (permalink / raw) To: Roland McGrath; +Cc: libc-alpha 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] assert.h: allow gcc to detect assert(a = 1) errors 2014-07-16 20:44 ` Jim Meyering @ 2016-11-24 2:22 ` Jim Meyering 2016-11-24 2:59 ` Zack Weinberg 2016-11-24 7:36 ` Florian Weimer 0 siblings, 2 replies; 17+ messages in thread From: Jim Meyering @ 2016-11-24 2:22 UTC (permalink / raw) To: Roland McGrath; +Cc: libc-alpha [-- Attachment #1: Type: text/plain, Size: 3534 bytes --] 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? [-- Attachment #2: glibc-assert--do-not-suppress-warning-about-accidental-assignment.diff --] [-- Type: text/plain, Size: 2072 bytes --] 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] assert.h: allow gcc to detect assert(a = 1) errors 2016-11-24 2:22 ` Jim Meyering @ 2016-11-24 2:59 ` Zack Weinberg 2016-11-24 13:13 ` Joseph Myers 2016-11-24 7:36 ` Florian Weimer 1 sibling, 1 reply; 17+ messages in thread From: Zack Weinberg @ 2016-11-24 2:59 UTC (permalink / raw) To: Jim Meyering; +Cc: Roland McGrath, libc-alpha On Wed, Nov 23, 2016 at 9:21 PM, Jim Meyering <jim@meyering.net> wrote: [...] Not commenting on anything else, but... > 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; } ... Would you please file a feature-request with the gcc people asking for something that turns __extension__ back off? It would be nice to be able to get this right even in __STRICT_ANSI__ mode. I'm imagining that your macro could look like #define assert(expr) \ __extension__ ({ \ if (__noextension__ (expr)) \ ; \ else __assert_failed (...); \ (void)0; \ }) (Make clear that the parentheses in __noextension__(...) need to not count for the purpose of -Wparentheses.) zw ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] assert.h: allow gcc to detect assert(a = 1) errors 2016-11-24 2:59 ` Zack Weinberg @ 2016-11-24 13:13 ` Joseph Myers 2016-11-26 6:36 ` Jim Meyering 0 siblings, 1 reply; 17+ messages in thread From: Joseph Myers @ 2016-11-24 13:13 UTC (permalink / raw) To: Zack Weinberg; +Cc: Jim Meyering, Roland McGrath, libc-alpha On Wed, 23 Nov 2016, Zack Weinberg wrote: > ... Would you please file a feature-request with the gcc people asking > for something that turns __extension__ back off? It would be nice to (Which I suggested in <https://gcc.gnu.org/ml/gcc/2001-04/msg00642.html> - note the point about counting nesting so this cancels exactly one enclosing __extension__.) -- Joseph S. Myers joseph@codesourcery.com ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] assert.h: allow gcc to detect assert(a = 1) errors 2016-11-24 13:13 ` Joseph Myers @ 2016-11-26 6:36 ` Jim Meyering 0 siblings, 0 replies; 17+ messages in thread From: Jim Meyering @ 2016-11-26 6:36 UTC (permalink / raw) To: Joseph Myers; +Cc: Zack Weinberg, Roland McGrath, libc-alpha On Thu, Nov 24, 2016 at 5:12 AM, Joseph Myers <joseph@codesourcery.com> wrote: > On Wed, 23 Nov 2016, Zack Weinberg wrote: > >> ... Would you please file a feature-request with the gcc people asking >> for something that turns __extension__ back off? It would be nice to > > (Which I suggested in <https://gcc.gnu.org/ml/gcc/2001-04/msg00642.html> - > note the point about counting nesting so this cancels exactly one > enclosing __extension__.) I made the feature request here: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=78539 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] assert.h: allow gcc to detect assert(a = 1) errors 2016-11-24 2:22 ` Jim Meyering 2016-11-24 2:59 ` Zack Weinberg @ 2016-11-24 7:36 ` Florian Weimer 2016-11-26 6:15 ` Jim Meyering 1 sibling, 1 reply; 17+ messages in thread From: Florian Weimer @ 2016-11-24 7:36 UTC (permalink / raw) To: Jim Meyering, Roland McGrath; +Cc: libc-alpha 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, Florian ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] assert.h: allow gcc to detect assert(a = 1) errors 2016-11-24 7:36 ` Florian Weimer @ 2016-11-26 6:15 ` Jim Meyering 2016-12-09 3:21 ` Jim Meyering 2016-12-14 9:13 ` Florian Weimer 0 siblings, 2 replies; 17+ messages in thread From: Jim Meyering @ 2016-11-26 6:15 UTC (permalink / raw) To: Florian Weimer; +Cc: Roland McGrath, libc-alpha 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] assert.h: allow gcc to detect assert(a = 1) errors 2016-11-26 6:15 ` Jim Meyering @ 2016-12-09 3:21 ` Jim Meyering 2016-12-14 5:28 ` Jim Meyering 2016-12-14 9:13 ` Florian Weimer 1 sibling, 1 reply; 17+ messages in thread From: Jim Meyering @ 2016-12-09 3:21 UTC (permalink / raw) To: Florian Weimer; +Cc: Roland McGrath, libc-alpha [-- Attachment #1: Type: text/plain, Size: 2091 bytes --] 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? [-- Attachment #2: glibc-assert--do-not-suppress-warning-about-accidental-assignment.diff --] [-- Type: text/plain, Size: 3132 bytes --] 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(-) 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) \ -- 2.9.3 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] assert.h: allow gcc to detect assert(a = 1) errors 2016-12-09 3:21 ` Jim Meyering @ 2016-12-14 5:28 ` Jim Meyering 0 siblings, 0 replies; 17+ messages in thread From: Jim Meyering @ 2016-12-14 5:28 UTC (permalink / raw) To: Florian Weimer; +Cc: Roland McGrath, libc-alpha 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? ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] assert.h: allow gcc to detect assert(a = 1) errors 2016-11-26 6:15 ` Jim Meyering 2016-12-09 3:21 ` Jim Meyering @ 2016-12-14 9:13 ` Florian Weimer 2016-12-15 0:25 ` Jim Meyering 1 sibling, 1 reply; 17+ messages in thread From: Florian Weimer @ 2016-12-14 9:13 UTC (permalink / raw) To: Jim Meyering; +Cc: Roland McGrath, libc-alpha 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? > 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, Florian ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] assert.h: allow gcc to detect assert(a = 1) errors 2016-12-14 9:13 ` Florian Weimer @ 2016-12-15 0:25 ` Jim Meyering 2016-12-16 12:15 ` Florian Weimer 2016-12-27 17:47 ` Andreas Schwab 0 siblings, 2 replies; 17+ messages in thread From: Jim Meyering @ 2016-12-15 0:25 UTC (permalink / raw) To: Florian Weimer; +Cc: Roland McGrath, libc-alpha [-- Attachment #1: Type: text/plain, Size: 2083 bytes --] 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. [-- Attachment #2: glibc-assert--do-not-suppress-warning-about-accidental-assignment.diff --] [-- Type: text/plain, Size: 2604 bytes --] 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] assert.h: allow gcc to detect assert(a = 1) errors 2016-12-15 0:25 ` Jim Meyering @ 2016-12-16 12:15 ` Florian Weimer 2016-12-18 9:53 ` Jim Meyering 2016-12-27 17:47 ` Andreas Schwab 1 sibling, 1 reply; 17+ messages in thread From: Florian Weimer @ 2016-12-16 12:15 UTC (permalink / raw) To: Jim Meyering; +Cc: Roland McGrath, libc-alpha On 12/15/2016 01:24 AM, Jim Meyering wrote: > 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. Thanks. I just noticed your fb.com address in the ChangeLog. Do you know if Facebook have filed paperwork with the FSF for glibc? Commit 7ee03f00188723a4de2b85021e511ced6d7fc4be was just two lines, and this commit here only adds only very few significant source lines, too, so we might get away without the paperwork, but this is probably the last such change we can accept. Anyway, please install if you have commit access. Thanks, Florian ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] assert.h: allow gcc to detect assert(a = 1) errors 2016-12-16 12:15 ` Florian Weimer @ 2016-12-18 9:53 ` Jim Meyering 0 siblings, 0 replies; 17+ messages in thread From: Jim Meyering @ 2016-12-18 9:53 UTC (permalink / raw) To: Florian Weimer; +Cc: Roland McGrath, libc-alpha On Fri, Dec 16, 2016 at 4:15 AM, Florian Weimer <fweimer@redhat.com> wrote: > On 12/15/2016 01:24 AM, Jim Meyering wrote: > >> 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. > > > Thanks. I just noticed your fb.com address in the ChangeLog. Do you know > if Facebook have filed paperwork with the FSF for glibc? Commit > 7ee03f00188723a4de2b85021e511ced6d7fc4be was just two lines, and this commit > here only adds only very few significant source lines, too, so we might get > away without the paperwork, but this is probably the last such change we can > accept. > > Anyway, please install if you have commit access. Thanks. Pushed. I'll add glibc to my list for the next Facebook copyright assignment. ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] assert.h: allow gcc to detect assert(a = 1) errors 2016-12-15 0:25 ` Jim Meyering 2016-12-16 12:15 ` Florian Weimer @ 2016-12-27 17:47 ` Andreas Schwab 2016-12-28 12:50 ` Florian Weimer 1 sibling, 1 reply; 17+ messages in thread From: Andreas Schwab @ 2016-12-27 17:47 UTC (permalink / raw) To: Jim Meyering; +Cc: Florian Weimer, Roland McGrath, libc-alpha On Dez 14 2016, Jim Meyering <jim@meyering.net> wrote: > 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 Missing colon. Andreas. -- Andreas Schwab, schwab@linux-m68k.org GPG Key fingerprint = 58CA 54C7 6D53 942B 1756 01D3 44D5 214B 8276 4ED5 "And now for something completely different." ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] assert.h: allow gcc to detect assert(a = 1) errors 2016-12-27 17:47 ` Andreas Schwab @ 2016-12-28 12:50 ` Florian Weimer 0 siblings, 0 replies; 17+ messages in thread From: Florian Weimer @ 2016-12-28 12:50 UTC (permalink / raw) To: Andreas Schwab, Jim Meyering; +Cc: Roland McGrath, libc-alpha On 12/27/2016 06:47 PM, Andreas Schwab wrote: > On Dez 14 2016, Jim Meyering <jim@meyering.net> wrote: > >> 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 > > Missing colon. I fixed this along with a couple of other ChangeLog typos. Thanks, Florian ^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2016-12-28 12:50 UTC | newest] Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2014-07-16 19:12 [PATCH] assert.h: allow gcc to detect assert(a = 1) errors Jim Meyering 2014-07-16 20:15 ` Roland McGrath 2014-07-16 20:44 ` Jim Meyering 2016-11-24 2:22 ` Jim Meyering 2016-11-24 2:59 ` Zack Weinberg 2016-11-24 13:13 ` Joseph Myers 2016-11-26 6:36 ` Jim Meyering 2016-11-24 7:36 ` Florian Weimer 2016-11-26 6:15 ` Jim Meyering 2016-12-09 3:21 ` Jim Meyering 2016-12-14 5:28 ` Jim Meyering 2016-12-14 9:13 ` Florian Weimer 2016-12-15 0:25 ` Jim Meyering 2016-12-16 12:15 ` Florian Weimer 2016-12-18 9:53 ` Jim Meyering 2016-12-27 17:47 ` Andreas Schwab 2016-12-28 12:50 ` Florian Weimer
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).