From patchwork Wed Jun 4 21:18:28 2014 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Marek Polacek X-Patchwork-Id: 356104 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from sourceware.org (server1.sourceware.org [209.132.180.131]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id F3D0814008F for ; Thu, 5 Jun 2014 07:18:47 +1000 (EST) DomainKey-Signature: a=rsa-sha1; c=nofws; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender:date :from:to:cc:subject:message-id:references:mime-version :content-type:in-reply-to; q=dns; s=default; b=uJYcCT5iQKx0pw0uj VSwT2Cwowaas4OP8Gg92p8v1psZQy5uomBRckbyI2uB1lEuKvpWx0DDIhGlv4qRc Y4SBaCxKoTEkDAMsMpbiwIY1z0FMR5gFaqWHp4qeTE/vNLuhYDmBluwVXdwT74QU CbaWkdFTwpy3lsE9PSshOiEbVk= DKIM-Signature: v=1; a=rsa-sha1; c=relaxed; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender:date :from:to:cc:subject:message-id:references:mime-version :content-type:in-reply-to; s=default; bh=uVbsNlcMQ6lWloCUDDzfcxw FsVk=; b=OfAY54d5Wnv+H4Zdf2Tm3RtQQrWfk9haPWNgVX3dQDwNyi7wO4B6jzA 0axZ8LPopvvdEDdnoxfZQHJmGMymo77oowMiwR5nwxmS3YsQ0kR/QZ87TGmcNwj6 tza5vDmApRlcP9oWw1jbpU4ebdpN1WQIAnZa3KckC9XsQD+l61jY= Received: (qmail 7240 invoked by alias); 4 Jun 2014 21:18:39 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Delivered-To: mailing list gcc-patches@gcc.gnu.org Received: (qmail 7227 invoked by uid 89); 4 Jun 2014 21:18:38 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-3.1 required=5.0 tests=AWL, BAYES_00, RP_MATCHES_RCVD, SPF_HELO_PASS, SPF_PASS autolearn=ham version=3.3.2 X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Wed, 04 Jun 2014 21:18:36 +0000 Received: from int-mx02.intmail.prod.int.phx2.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id s54LIXEF032666 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Wed, 4 Jun 2014 17:18:33 -0400 Received: from redhat.com (ovpn-116-106.ams2.redhat.com [10.36.116.106]) by int-mx02.intmail.prod.int.phx2.redhat.com (8.13.8/8.13.8) with ESMTP id s54LITS5016605 (version=TLSv1/SSLv3 cipher=AES128-SHA bits=128 verify=NO); Wed, 4 Jun 2014 17:18:31 -0400 Date: Wed, 4 Jun 2014 23:18:28 +0200 From: Marek Polacek To: Jakub Jelinek Cc: Jason Merrill , GCC Patches , "Joseph S. Myers" Subject: Re: [C/C++ PATCH] Add -Wlogical-not-parentheses (PR c/49706) Message-ID: <20140604211828.GK29196@redhat.com> References: <538C8558.7010404@redhat.com> <20140602151732.GA11694@redhat.com> <538C9FD2.7050404@redhat.com> <20140602162300.GD11694@redhat.com> <538CA6E4.2040401@redhat.com> <20140602163606.GO10386@tucnak.redhat.com> <20140602170457.GA27647@redhat.com> <538CB97D.8040904@redhat.com> <20140604205643.GJ29196@redhat.com> <20140604210210.GB10386@tucnak.redhat.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20140604210210.GB10386@tucnak.redhat.com> User-Agent: Mutt/1.5.23 (2014-03-12) On Wed, Jun 04, 2014 at 11:02:10PM +0200, Jakub Jelinek wrote: > On Wed, Jun 04, 2014 at 10:56:43PM +0200, Marek Polacek wrote: > > > +/* Warn about logical not used on the left hand side operand of a comparison. > > + This function assumes that the LHS is inside of TRUTH_NOT_EXPR. > > + Do not warn if the LHS or RHS is of a boolean or a vector type. */ > > + > > +void > > +warn_logical_not_parentheses (location_t location, enum tree_code code, > > + tree lhs, tree rhs) > > +{ > > + if (TREE_TYPE (lhs) == NULL_TREE > > + || TREE_TYPE (rhs) == NULL_TREE) > > + ; > > + else if (TREE_CODE_CLASS (code) != tcc_comparison > > Shouldn't this test be done first? I mean, if type is NULL on lhs or rhs, > you'll warn even when code is not comparison... Obviously it's too late for me :^). Hopefully fixed... 2014-06-04 Marek Polacek PR c/49706 * doc/invoke.texi: Document -Wlogical-not-parentheses. c-family/ * c-common.c (warn_logical_not_parentheses): New function. * c-common.h (warn_logical_not_parentheses): Declare. * c.opt (Wlogical-not-parentheses): New option. c/ * c-typeck.c (parser_build_binary_op): Warn when logical not is used on the left hand side operand of a comparison. cp/ * parser.c (cp_parser_binary_expression): Warn when logical not is used on the left hand side operand of a comparison. testsuite/ * c-c++-common/pr49706.c: New test. Marek diff --git gcc/c-family/c-common.c gcc/c-family/c-common.c index 07a1798..fb6c612 100644 --- gcc/c-family/c-common.c +++ gcc/c-family/c-common.c @@ -1722,6 +1722,29 @@ warn_logical_operator (location_t location, enum tree_code code, tree type, } } +/* Warn about logical not used on the left hand side operand of a comparison. + This function assumes that the LHS is inside of TRUTH_NOT_EXPR. + Do not warn if the LHS or RHS is of a boolean or a vector type. */ + +void +warn_logical_not_parentheses (location_t location, enum tree_code code, + tree lhs, tree rhs) +{ + if (TREE_CODE_CLASS (code) != tcc_comparison) + return; + if (TREE_TYPE (lhs) == NULL_TREE + || TREE_TYPE (rhs) == NULL_TREE) + ; + else if (TREE_CODE (TREE_TYPE (lhs)) == BOOLEAN_TYPE + || TREE_CODE (TREE_TYPE (rhs)) == BOOLEAN_TYPE + || VECTOR_TYPE_P (TREE_TYPE (lhs)) + || VECTOR_TYPE_P (TREE_TYPE (rhs))) + return; + + warning_at (location, OPT_Wlogical_not_parentheses, + "logical not is only applied to the left hand side of " + "comparison"); +} /* Warn if EXP contains any computations whose results are not used. Return true if a warning is printed; false otherwise. LOCUS is the diff --git gcc/c-family/c-common.h gcc/c-family/c-common.h index 0d34004..83d5dee 100644 --- gcc/c-family/c-common.h +++ gcc/c-family/c-common.h @@ -772,6 +772,8 @@ extern void overflow_warning (location_t, tree); extern bool warn_if_unused_value (const_tree, location_t); extern void warn_logical_operator (location_t, enum tree_code, tree, enum tree_code, tree, enum tree_code, tree); +extern void warn_logical_not_parentheses (location_t, enum tree_code, tree, + tree); extern void check_main_parameter_types (tree decl); extern bool c_determine_visibility (tree); extern bool vector_types_compatible_elements_p (tree, tree); diff --git gcc/c-family/c.opt gcc/c-family/c.opt index 5d36a80..76e67d7 100644 --- gcc/c-family/c.opt +++ gcc/c-family/c.opt @@ -490,6 +490,10 @@ Wlogical-op C ObjC C++ ObjC++ Var(warn_logical_op) Init(0) Warning Warn when a logical operator is suspiciously always evaluating to true or false +Wlogical-not-parentheses +C ObjC C++ ObjC++ Var(warn_logical_not_paren) Warning +Warn when logical not is used on the left hand side operand of a comparison + Wlong-long C ObjC C++ ObjC++ Var(warn_long_long) Init(-1) Warning Do not warn about using \"long long\" when -pedantic diff --git gcc/c/c-typeck.c gcc/c/c-typeck.c index a98ce07..e0d3fde 100644 --- gcc/c/c-typeck.c +++ gcc/c/c-typeck.c @@ -3401,6 +3401,10 @@ parser_build_binary_op (location_t location, enum tree_code code, warn_logical_operator (location, code, TREE_TYPE (result.value), code1, arg1.value, code2, arg2.value); + if (warn_logical_not_paren + && code1 == TRUTH_NOT_EXPR) + warn_logical_not_parentheses (location, code, arg1.value, arg2.value); + /* Warn about comparisons against string literals, with the exception of testing for equality or inequality of a string literal with NULL. */ if (code == EQ_EXPR || code == NE_EXPR) diff --git gcc/cp/parser.c gcc/cp/parser.c index 2591ae5..60e6cda 100644 --- gcc/cp/parser.c +++ gcc/cp/parser.c @@ -7883,6 +7883,8 @@ cp_parser_binary_expression (cp_parser* parser, bool cast_p, enum tree_code rhs_type; enum cp_parser_prec new_prec, lookahead_prec; tree overload; + bool parenthesized_not_lhs_warn + = cp_lexer_next_token_is (parser->lexer, CPP_NOT); /* Parse the first expression. */ current.lhs = cp_parser_cast_expression (parser, /*address_p=*/false, @@ -7985,6 +7987,11 @@ cp_parser_binary_expression (cp_parser* parser, bool cast_p, else if (current.tree_type == TRUTH_ORIF_EXPR) c_inhibit_evaluation_warnings -= current.lhs == truthvalue_true_node; + if (warn_logical_not_paren + && parenthesized_not_lhs_warn) + warn_logical_not_parentheses (current.loc, current.tree_type, + TREE_OPERAND (current.lhs, 0), rhs); + overload = NULL; /* ??? Currently we pass lhs_type == ERROR_MARK and rhs_type == ERROR_MARK for everything that is not a binary expression. diff --git gcc/doc/invoke.texi gcc/doc/invoke.texi index 1c2e079..789d702 100644 --- gcc/doc/invoke.texi +++ gcc/doc/invoke.texi @@ -256,7 +256,7 @@ Objective-C and Objective-C++ Dialects}. -Winit-self -Winline -Wmaybe-uninitialized @gol -Wno-int-to-pointer-cast -Wno-invalid-offsetof @gol -Winvalid-pch -Wlarger-than=@var{len} -Wunsafe-loop-optimizations @gol --Wlogical-op -Wlong-long @gol +-Wlogical-op -Wlogical-not-parentheses -Wlong-long @gol -Wmain -Wmaybe-uninitialized -Wmissing-braces -Wmissing-field-initializers @gol -Wmissing-include-dirs @gol -Wno-multichar -Wnonnull -Wno-overflow -Wopenmp-simd @gol @@ -4704,6 +4704,24 @@ Warn about suspicious uses of logical operators in expressions. This includes using logical operators in contexts where a bit-wise operator is likely to be expected. +@item -Wlogical-not-parentheses +@opindex Wlogical-not-parentheses +@opindex Wno-logical-not-parentheses +Warn about logical not used on the left hand side operand of a comparison. +This option does not warn if the LHS or RHS operand is of a boolean or +a vector type. Its purpose is to detect suspicious code like the following: +@smallexample +int a; +... +if (!a > 1) @{ ... @} +@end smallexample + +It is possible to suppress the warning by wrapping the LHS into +parentheses: +@smallexample +if ((!a) > 1) @{ ... @} +@end smallexample + @item -Waggregate-return @opindex Waggregate-return @opindex Wno-aggregate-return diff --git gcc/testsuite/c-c++-common/pr49706.c gcc/testsuite/c-c++-common/pr49706.c index e69de29..615f3e4 100644 --- gcc/testsuite/c-c++-common/pr49706.c +++ gcc/testsuite/c-c++-common/pr49706.c @@ -0,0 +1,102 @@ +/* PR c/49706 */ +/* { dg-do compile } */ +/* { dg-options "-Wlogical-not-parentheses" } */ + +#ifndef __cplusplus +#define bool _Bool +#endif +enum E { A, B }; +bool b; +extern enum E foo_e (void); +extern bool foo_b (void); +extern int foo_i (void); + +#ifdef __cplusplus +template bool f1(T t, U u) { return (!t == u); } /* { dg-warning "logical not is only applied to the left hand side of comparison" "" { target c++ } 15 } */ +template bool f2(T t, U u) { return ((!t) == u); } +template bool f3(T t, U u) { return (!g(t) == u); } /* { dg-warning "logical not is only applied to the left hand side of comparison" "" { target c++ } 17 } */ +template bool f4(T t, U u) { return ((!g(t)) == u); } +#endif + +void +fn1 (int i1, int i2, bool b1, bool b2) +{ + b = !i1 == i2; /* { dg-warning "logical not is only applied to the left hand side of comparison" } */ + b = !i1 != i2; /* { dg-warning "logical not is only applied to the left hand side of comparison" } */ + b = !i1 < i2; /* { dg-warning "logical not is only applied to the left hand side of comparison" } */ + b = !i1 > i2; /* { dg-warning "logical not is only applied to the left hand side of comparison" } */ + b = !i1 <= i2; /* { dg-warning "logical not is only applied to the left hand side of comparison" } */ + b = !i1 >= i2; /* { dg-warning "logical not is only applied to the left hand side of comparison" } */ + + b = i1 == i2; + b = i1 != i2; + b = i1 < i2; + b = i1 > i2; + b = i1 <= i2; + b = i1 >= i2; + + /* Parens suppress the warning. */ + b = (!i1) == i2; + b = (!i1) != i2; + b = (!i1) < i2; + b = (!i1) > i2; + b = (!i1) <= i2; + b = (!i1) >= i2; + + /* ...but not these parens. */ + b = (!i1 == i2); /* { dg-warning "logical not is only applied to the left hand side of comparison" } */ + b = (!i1 != i2); /* { dg-warning "logical not is only applied to the left hand side of comparison" } */ + b = (!i1 < i2); /* { dg-warning "logical not is only applied to the left hand side of comparison" } */ + b = (!i1 > i2); /* { dg-warning "logical not is only applied to the left hand side of comparison" } */ + b = (!i1 <= i2); /* { dg-warning "logical not is only applied to the left hand side of comparison" } */ + b = (!i1 >= i2); /* { dg-warning "logical not is only applied to the left hand side of comparison" } */ + + b = !b1 == b2; + b = !b1 != b2; + b = !b1 < b2; + b = !b1 > b2; + b = !b1 <= b2; + b = !b1 >= b2; + + b = !foo_i () == i1; /* { dg-warning "logical not is only applied to the left hand side of comparison" } */ + b = (!foo_i ()) == i1; + b = !foo_b () == b1; + + b = !!i1 == i2; /* { dg-warning "logical not is only applied to the left hand side of comparison" } */ + b = !!i1 != i2; /* { dg-warning "logical not is only applied to the left hand side of comparison" } */ + b = !!i1 < i2; /* { dg-warning "logical not is only applied to the left hand side of comparison" } */ + b = !!i1 > i2; /* { dg-warning "logical not is only applied to the left hand side of comparison" } */ + b = !!i1 <= i2; /* { dg-warning "logical not is only applied to the left hand side of comparison" } */ + b = !!i1 >= i2; /* { dg-warning "logical not is only applied to the left hand side of comparison" } */ + b = !!foo_i () == i1; /* { dg-warning "logical not is only applied to the left hand side of comparison" } */ + + /* Be careful here. */ + b = (i1 == 0) != 0; + b = (i1 == 0) == 0; + b = (i1 != 0) != 0; + b = (i1 != 0) == 0; +} + +void +fn2 (enum E e) +{ + b = e == B; + b = e == foo_e (); + b = foo_e () == A; + b = foo_e () == foo_e (); + + b = !e == A; /* { dg-warning "logical not is only applied to the left hand side of comparison" } */ + b = !e == foo_e (); /* { dg-warning "logical not is only applied to the left hand side of comparison" } */ + b = !foo_e () == A; /* { dg-warning "logical not is only applied to the left hand side of comparison" } */ + b = !foo_e () == foo_e (); /* { dg-warning "logical not is only applied to the left hand side of comparison" } */ + + b = !(e == A); + b = !(e == foo_e ()); + b = !(foo_e () == A); + b = !(foo_e () == foo_e ()); + + b = (!e) == A; + b = (!e) == foo_e (); + b = (!foo_e ()) == A; + b = (!foo_e ()) == foo_e (); +}