From patchwork Thu Jun 24 12:57:44 2010 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Andi Kleen X-Patchwork-Id: 56787 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]) by ozlabs.org (Postfix) with SMTP id 8C465B6EF7 for ; Thu, 24 Jun 2010 22:57:58 +1000 (EST) Received: (qmail 8317 invoked by alias); 24 Jun 2010 12:57:55 -0000 Received: (qmail 8296 invoked by uid 22791); 24 Jun 2010 12:57:53 -0000 X-SWARE-Spam-Status: No, hits=-2.1 required=5.0 tests=AWL, BAYES_00, T_RP_MATCHES_RCVD X-Spam-Check-By: sourceware.org Received: from one.firstfloor.org (HELO one.firstfloor.org) (213.235.205.2) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Thu, 24 Jun 2010 12:57:47 +0000 Received: from basil.firstfloor.org (p5B3CB3C4.dip0.t-ipconnect.de [91.60.179.196]) by one.firstfloor.org (Postfix) with ESMTP id EC0561A980C4; Thu, 24 Jun 2010 14:57:44 +0200 (CEST) Received: by basil.firstfloor.org (Postfix, from userid 1000) id 78242B1F52; Thu, 24 Jun 2010 14:57:44 +0200 (CEST) Date: Thu, 24 Jun 2010 14:57:44 +0200 From: Andi Kleen To: Jakub Jelinek Cc: Andi Kleen , Manuel =?iso-8859-1?B?TPNwZXotSWLh8WV6?= , Paolo Carlini , Richard Guenther , gcc-patches@gcc.gnu.org Subject: Re: [PATCH] Warn for dangerous use of omitted middle operand in ?: Message-ID: <20100624125744.GL578@basil.fritz.box> References: <20100531124323.GB10766@basil.fritz.box> <20100531131516.GI10293@tyan-ft48-01.lab.bos.redhat.com> <20100531142050.GC10766@basil.fritz.box> <4C03C84F.2050005@oracle.com> <20100531154834.GD10766@basil.fritz.box> <20100603091818.GA5034@basil.fritz.box> <20100624112412.GE578@basil.fritz.box> <20100624122606.GI578@basil.fritz.box> <20100624123735.GE12443@tyan-ft48-01.lab.bos.redhat.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20100624123735.GE12443@tyan-ft48-01.lab.bos.redhat.com> User-Agent: Mutt/1.5.20 (2009-06-14) 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 On Thu, Jun 24, 2010 at 02:37:35PM +0200, Jakub Jelinek wrote: > On Thu, Jun 24, 2010 at 02:26:06PM +0200, Andi Kleen wrote: > > > > gcc/cp: > > 2010-05-31 Andi Kleen > > * cp/parser.cx: (cp_parser_question_colon_clause): > > s/x:// above New patch appended. > > > Switch to use cp_lexer_peek_token. > > Call warn_for_omitted_condop. Call pedwarn for omitted > > middle operand. > > > > +/* Warn for A ?: C expressions (with B omitted) where A is a boolean > > + expression, because B will always be true. */ > > + > > +void > > +warn_for_omitted_condop (location_t location, tree cond) > > +{ > > + if (truth_value_p (TREE_CODE (cond))) > > Shouldn't this warn also for > || (DECL_P (cond) > && TREE_CODE (TREE_TYPE (cond)) == BOOLEAN_TYPE) > (or perhaps for all of > || TREE_CODE (TREE_TYPE (cond)) == BOOLEAN_TYPE > ? Hmm I'm not sure that's really a broken case, I assume this can be used without mistake. Simply reusing a single variable is the main use case of the omitted middle op I think. I tried to be conservative here. > > bool a; > > int foo () > { > return a ?: 2; Wouldn't that give a type mismatch warning anyways? Hmm or maybe not or maybe it should? -Andi ---- gcc/ 2010-05-31 Andi Kleen * c-parser.c (c_parser_conditional_expression): Call warn_for_omitted_condop. * c-common.c (warn_for_omitted_condop): Add. * c-common.h (warn_for_omitted_condop): Add prototype. * doc/invoke.texi: Document omitted condop warning. testsuite/ 2010-05-31 Andi Kleen * gcc.dg/warn-omitted-condop.c: Add. * g++.dg/warn-omitted-condop.C: Add. gcc/cp: 2010-05-31 Andi Kleen * cp/parser.c: (cp_parser_question_colon_clause): Switch to use cp_lexer_peek_token. Call warn_for_omitted_condop. Call pedwarn for omitted middle operand. diff --git a/gcc/c-family/c-common.c b/gcc/c-family/c-common.c index 86c3802..14675bc 100644 --- a/gcc/c-family/c-common.c +++ b/gcc/c-family/c-common.c @@ -8405,6 +8405,18 @@ fold_offsetof (tree expr, tree stop_ref) return convert (size_type_node, fold_offsetof_1 (expr, stop_ref)); } +/* Warn for A ?: C expressions (with B omitted) where A is a boolean + expression, because B will always be true. */ + +void +warn_for_omitted_condop (location_t location, tree cond) +{ + if (truth_value_p (TREE_CODE (cond))) + warning_at (location, OPT_Wparentheses, + "the omitted middle operand in ?: will always be %, " + "suggest explicit middle operand"); +} + /* Print an error message for an invalid lvalue. USE says how the lvalue is being used and so selects the error message. */ diff --git a/gcc/c-family/c-common.h b/gcc/c-family/c-common.h index 6948b82..5784746 100644 --- a/gcc/c-family/c-common.h +++ b/gcc/c-family/c-common.h @@ -736,6 +736,7 @@ extern void c_parse_file (void); /* This is misnamed, it actually performs end-of-compilation processing. */ extern void finish_file (void); +extern void warn_for_omitted_condop (location_t, tree); /* These macros provide convenient access to the various _STMT nodes. */ diff --git a/gcc/c-parser.c b/gcc/c-parser.c index 04506df..ba18478 100644 --- a/gcc/c-parser.c +++ b/gcc/c-parser.c @@ -4795,7 +4795,7 @@ static struct c_expr c_parser_conditional_expression (c_parser *parser, struct c_expr *after) { struct c_expr cond, exp1, exp2, ret; - location_t cond_loc, colon_loc; + location_t cond_loc, colon_loc, middle_loc; gcc_assert (!after || c_dialect_objc ()); @@ -4809,8 +4809,11 @@ c_parser_conditional_expression (c_parser *parser, struct c_expr *after) if (c_parser_next_token_is (parser, CPP_COLON)) { tree eptype = NULL_TREE; - pedwarn (c_parser_peek_token (parser)->location, OPT_pedantic, + + middle_loc = c_parser_peek_token (parser)->location; + pedwarn (middle_loc, OPT_pedantic, "ISO C forbids omitting the middle term of a ?: expression"); + warn_for_omitted_condop (middle_loc, cond.value); if (TREE_CODE (cond.value) == EXCESS_PRECISION_EXPR) { eptype = TREE_TYPE (cond.value); diff --git a/gcc/cp/parser.c b/gcc/cp/parser.c index c6f8d7e..4e4db2d 100644 --- a/gcc/cp/parser.c +++ b/gcc/cp/parser.c @@ -6815,15 +6815,20 @@ cp_parser_question_colon_clause (cp_parser* parser, tree logical_or_expr) { tree expr; tree assignment_expr; + struct cp_token *token; /* Consume the `?' token. */ cp_lexer_consume_token (parser->lexer); + token = cp_lexer_peek_token (parser->lexer); if (cp_parser_allow_gnu_extensions_p (parser) - && cp_lexer_next_token_is (parser->lexer, CPP_COLON)) + && token->type == CPP_COLON) { + pedwarn (token->location, OPT_pedantic, + "ISO C++ does not allow ?: with omitted middle operand"); /* Implicit true clause. */ expr = NULL_TREE; c_inhibit_evaluation_warnings += logical_or_expr == truthvalue_true_node; + warn_for_omitted_condop (token->location, logical_or_expr); } else { diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi index 88240fc..3eae7d7 100644 --- a/gcc/doc/invoke.texi +++ b/gcc/doc/invoke.texi @@ -3275,6 +3275,12 @@ look like this: @end group @end smallexample +Also warn for dangerous uses of the +?: with omitted middle operand GNU extension. When the condition +in the ?: operator is a boolean expression the omitted value will +be always 1. Often the user expects it to be a value computed +inside the conditional expression instead. + This warning is enabled by @option{-Wall}. @item -Wsequence-point diff --git a/gcc/testsuite/g++.dg/warn/warn-omitted-condop.C b/gcc/testsuite/g++.dg/warn/warn-omitted-condop.C new file mode 100644 index 0000000..9fda387 --- /dev/null +++ b/gcc/testsuite/g++.dg/warn/warn-omitted-condop.C @@ -0,0 +1,58 @@ +/* { dg-options "-Wparentheses" } */ + +extern void f2 (int); + +void bar (int x, int y, int z) +{ +#define T(op) f2 (x op y ? : 1) +#define T2(op) f2 (x op y ? 2 : 1) + + T(<); /* { dg-warning "omitted middle operand" } */ + T(>); /* { dg-warning "omitted middle operand" } */ + T(<=); /* { dg-warning "omitted middle operand" } */ + T(>=); /* { dg-warning "omitted middle operand" } */ + T(==); /* { dg-warning "omitted middle operand" } */ + T(!=); /* { dg-warning "omitted middle operand" } */ + T(||); /* { dg-warning "omitted middle operand" } */ + T(&&); /* { dg-warning "omitted middle operand" } */ + f2 (!x ? : 1); /* { dg-warning "omitted middle operand" } */ + T2(<); /* { dg-bogus "omitted middle operand" } */ + T2(>); /* { dg-bogus "omitted middle operand" } */ + T2(==); /* { dg-bogus "omitted middle operand" } */ + T2(||); /* { dg-bogus "omitted middle operand" } */ + T2(&&); /* { dg-bogus "omitted middle operand" } */ + T(+); /* { dg-bogus "omitted middle operand" } */ + T(-); /* { dg-bogus "omitted middle operand" } */ + T(*); /* { dg-bogus "omitted middle operand" } */ + T(/); /* { dg-bogus "omitted middle operand" } */ + T(^); /* { dg-bogus "omitted middle operand" } */ +} +/* { dg-options "-Wparentheses" } */ + +extern void f2 (int); + +void bar (int x, int y, int z) +{ +#define T(op) f2 (x op y ? : 1) +#define T2(op) f2 (x op y ? 2 : 1) + + T(<); /* { dg-warning "omitted middle operand" } */ + T(>); /* { dg-warning "omitted middle operand" } */ + T(<=); /* { dg-warning "omitted middle operand" } */ + T(>=); /* { dg-warning "omitted middle operand" } */ + T(==); /* { dg-warning "omitted middle operand" } */ + T(!=); /* { dg-warning "omitted middle operand" } */ + T(||); /* { dg-warning "omitted middle operand" } */ + T(&&); /* { dg-warning "omitted middle operand" } */ + f2 (!x ? : 1); /* { dg-warning "omitted middle operand" } */ + T2(<); /* { dg-bogus "omitted middle operand" } */ + T2(>); /* { dg-bogus "omitted middle operand" } */ + T2(==); /* { dg-bogus "omitted middle operand" } */ + T2(||); /* { dg-bogus "omitted middle operand" } */ + T2(&&); /* { dg-bogus "omitted middle operand" } */ + T(+); /* { dg-bogus "omitted middle operand" } */ + T(-); /* { dg-bogus "omitted middle operand" } */ + T(*); /* { dg-bogus "omitted middle operand" } */ + T(/); /* { dg-bogus "omitted middle operand" } */ + T(^); /* { dg-bogus "omitted middle operand" } */ +} diff --git a/gcc/testsuite/gcc.dg/warn-omitted-condop.c b/gcc/testsuite/gcc.dg/warn-omitted-condop.c new file mode 100644 index 0000000..9fda387 --- /dev/null +++ b/gcc/testsuite/gcc.dg/warn-omitted-condop.c @@ -0,0 +1,58 @@ +/* { dg-options "-Wparentheses" } */ + +extern void f2 (int); + +void bar (int x, int y, int z) +{ +#define T(op) f2 (x op y ? : 1) +#define T2(op) f2 (x op y ? 2 : 1) + + T(<); /* { dg-warning "omitted middle operand" } */ + T(>); /* { dg-warning "omitted middle operand" } */ + T(<=); /* { dg-warning "omitted middle operand" } */ + T(>=); /* { dg-warning "omitted middle operand" } */ + T(==); /* { dg-warning "omitted middle operand" } */ + T(!=); /* { dg-warning "omitted middle operand" } */ + T(||); /* { dg-warning "omitted middle operand" } */ + T(&&); /* { dg-warning "omitted middle operand" } */ + f2 (!x ? : 1); /* { dg-warning "omitted middle operand" } */ + T2(<); /* { dg-bogus "omitted middle operand" } */ + T2(>); /* { dg-bogus "omitted middle operand" } */ + T2(==); /* { dg-bogus "omitted middle operand" } */ + T2(||); /* { dg-bogus "omitted middle operand" } */ + T2(&&); /* { dg-bogus "omitted middle operand" } */ + T(+); /* { dg-bogus "omitted middle operand" } */ + T(-); /* { dg-bogus "omitted middle operand" } */ + T(*); /* { dg-bogus "omitted middle operand" } */ + T(/); /* { dg-bogus "omitted middle operand" } */ + T(^); /* { dg-bogus "omitted middle operand" } */ +} +/* { dg-options "-Wparentheses" } */ + +extern void f2 (int); + +void bar (int x, int y, int z) +{ +#define T(op) f2 (x op y ? : 1) +#define T2(op) f2 (x op y ? 2 : 1) + + T(<); /* { dg-warning "omitted middle operand" } */ + T(>); /* { dg-warning "omitted middle operand" } */ + T(<=); /* { dg-warning "omitted middle operand" } */ + T(>=); /* { dg-warning "omitted middle operand" } */ + T(==); /* { dg-warning "omitted middle operand" } */ + T(!=); /* { dg-warning "omitted middle operand" } */ + T(||); /* { dg-warning "omitted middle operand" } */ + T(&&); /* { dg-warning "omitted middle operand" } */ + f2 (!x ? : 1); /* { dg-warning "omitted middle operand" } */ + T2(<); /* { dg-bogus "omitted middle operand" } */ + T2(>); /* { dg-bogus "omitted middle operand" } */ + T2(==); /* { dg-bogus "omitted middle operand" } */ + T2(||); /* { dg-bogus "omitted middle operand" } */ + T2(&&); /* { dg-bogus "omitted middle operand" } */ + T(+); /* { dg-bogus "omitted middle operand" } */ + T(-); /* { dg-bogus "omitted middle operand" } */ + T(*); /* { dg-bogus "omitted middle operand" } */ + T(/); /* { dg-bogus "omitted middle operand" } */ + T(^); /* { dg-bogus "omitted middle operand" } */ +}