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

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

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
  

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.
  

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.
  

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