Re: [PATCH] assert.h: allow gcc to detect assert(a = 1) errors

[Jim Meyering <jim at meyering dot net>]

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: glibc-assert--do-not-suppress-warning-about-accidental-assignment.diff
Description: Text document