From patchwork Fri Dec 9 03:17:57 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Jim Meyering X-Patchwork-Id: 18310 Received: (qmail 9070 invoked by alias); 9 Dec 2016 03:21:19 -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 6857 invoked by uid 89); 9 Dec 2016 03:18:30 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.4 required=5.0 tests=AWL, BAYES_00, FREEMAIL_FROM, RCVD_IN_DNSWL_NONE, RCVD_IN_SORBS_SPAM, SPF_PASS, URIBL_RED autolearn=no version=3.3.2 spammy=Agreed, Here's, Heres, jun X-HELO: mail-io0-f194.google.com X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:mime-version:sender:in-reply-to:references:from :date:message-id:subject:to:cc; bh=j2V5nReWNYnEeerruD0IU2zg7CyYuC7PIrMSDH6EPhI=; b=EDFtPhy4ap1drsh2D3jQdLa7GtVkSx1kG8i+n8xv6M1qazQv8+ZdB5yYp5sMdZ19NE Z9hePxvs/SLjNyTM6NhqObXWH0PKZDMk7VTiabV6AY7+DpH9ZzgUAxKrOYpC47OSMV7Q MGzE59MHJnZZkG4fnK4YLpad5ZFY/V/3zoa/BmymwVSZ3j7a6k/wwYsBPdXmkJAq1dK0 6p4nTgyus/I26bW/5iEanacFDTHfpPtDHwUS2wmD5n5b1xn6VwvLAy/ZUDq2mho0wUYW GSyeB/lDjs34pIgqO2xHSGAQl6j4WLh43AJkM2JPiZtskD0w5S1XnVcasesrGorTDb6P dO1A== X-Gm-Message-State: AKaTC02wMmydT9EapqtYXXSG+CFTNUkv4bXGY6E2hD14Uw8kLlIcsZjKi90vKny8jrht2g2nv+t7peGJaql61w== X-Received: by 10.107.149.144 with SMTP id x138mr67371280iod.23.1481253498680; Thu, 08 Dec 2016 19:18:18 -0800 (PST) MIME-Version: 1.0 In-Reply-To: References: <1405537923-28692-1-git-send-email-jim@meyering.net> <20140716201505.34FF22C398F@topped-with-meat.com> <93a7b09e-70b9-d11e-bfb5-e54e751c8db5@redhat.com> From: Jim Meyering Date: Thu, 8 Dec 2016 19:17:57 -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 Fri, Nov 25, 2016 at 10:14 PM, Jim Meyering wrote: > On Wed, Nov 23, 2016 at 11:36 PM, Florian Weimer 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 > > 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? From 0954feae6411cc0de5f5cb6c7e007b972388139f 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 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 + + 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 * 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) \