From patchwork Thu Dec 15 00:24:30 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jim Meyering X-Patchwork-Id: 18469 Received: (qmail 100028 invoked by alias); 15 Dec 2016 00:25:05 -0000 Mailing-List: contact libc-alpha-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: libc-alpha-owner@sourceware.org Delivered-To: mailing list libc-alpha@sourceware.org Received: (qmail 100014 invoked by uid 89); 15 Dec 2016 00:25:03 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.9 required=5.0 tests=AWL, BAYES_00, FREEMAIL_FROM, RCVD_IN_DNSWL_NONE, SPF_PASS, URIBL_RED autolearn=ham version=3.3.2 spammy=H*Ad:U*roland, __END_DECLS, __end_decls, H*F:U*jim X-HELO: mail-io0-f194.google.com X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:sender:in-reply-to:references:from :date:message-id:subject:to:cc; bh=P/ODz9zBZAS68WJnIvTiyw/TXzaZKbu7jZwZJWdkaQw=; b=EL3NAO1htchSdnuS3ov01ZI/5B38GXGYVUbcaDmQ3Eo0y76ZHOHQRQUK/2zOPI8U7E 1Otm0jFlBWH66DvKqPScw+mWEgqzrDQMLMcNstEIf/bowAX8dwWppIvOXJse0zeqbsXG oeAQtd7hktPkwP4HKPKZ5EXJZDEETQ9GmfEO0ctePKc87QWUYuHPHZROJxLavXpwZAAw /FliQNAPPdH0Nw6now0X7Xk3JFwhxOjzvvSO5yoc3Xpf52h25y3cXbwcP8LLzafA16ML xibUFDgLR5NBoDqN7cN1KL99EtA6kyOtlxuwgiD9p+32A+rjmPMrS7zqrsHjYBHQvDis zB9Q== X-Gm-Message-State: AKaTC01B4kgivcGF7wzpCjD9PUCESFOgpoe6A43ddqm0W08W3ABWjF5ogJZzRadW1UC3r5oEUzSV2m65JuxyxQ== X-Received: by 10.107.10.11 with SMTP id u11mr31896ioi.29.1481761491971; Wed, 14 Dec 2016 16:24:51 -0800 (PST) MIME-Version: 1.0 In-Reply-To: <42486853-b892-20d1-d8e5-c74548369e2e@redhat.com> References: <1405537923-28692-1-git-send-email-jim@meyering.net> <20140716201505.34FF22C398F@topped-with-meat.com> <93a7b09e-70b9-d11e-bfb5-e54e751c8db5@redhat.com> <42486853-b892-20d1-d8e5-c74548369e2e@redhat.com> From: Jim Meyering Date: Wed, 14 Dec 2016 16:24:30 -0800 Message-ID: Subject: Re: [PATCH] assert.h: allow gcc to detect assert(a = 1) errors To: Florian Weimer Cc: Roland McGrath , libc-alpha On Wed, Dec 14, 2016 at 1:13 AM, Florian Weimer wrote: > On 11/26/2016 07:14 AM, Jim Meyering wrote: > >> Here's a proposed ChangeLog entry: >> >> 2016-11-25 Jim Meyering >> >> 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 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 + + 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 [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) \