diff mbox

Warn for dangerous use of omitted middle operand in ?:

Message ID 20100624122606.GI578@basil.fritz.box
State New
Headers show

Commit Message

Andi Kleen June 24, 2010, 12:26 p.m. UTC 
On Thu, Jun 24, 2010 at 01:50:59PM +0200, Manuel López-Ibáñez wrote:
> On 24 June 2010 13:24, Andi Kleen <andi@firstfloor.org> wrote:
> > On Thu, Jun 03, 2010 at 11:18:18AM +0200, Andi Kleen wrote:
> >>
> >> Ping, can someone commit this please?
> >
> > *ping^2*
> 
> If the patch is approved, please rediff against a recent revision,
> send it with the changelog (or provide me with a link to the
> changelog) and I will commit it for you.

Here's a rediffed patch. 
-Andi

gcc/
2010-05-31  Andi Kleen  <ak@linux.intel.com>
	* 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  <ak@linux.intel.com>
        * gcc.dg/warn-omitted-condop.c: Add.
        * g++.dg/warn-omitted-condop.C: Add.

gcc/cp:
2010-05-31  Andi Kleen  <ak@linux.intel.com>
	* cp/parser.cx: (cp_parser_question_colon_clause):
        Switch to use cp_lexer_peek_token.
        Call warn_for_omitted_condop. Call pedwarn for omitted
        middle operand.

Comments

Jakub Jelinek June 24, 2010, 12:37 p.m. UTC | #1 
On Thu, Jun 24, 2010 at 02:26:06PM +0200, Andi Kleen wrote:
> 
> gcc/cp:
> 2010-05-31  Andi Kleen  <ak@linux.intel.com>
> 	* cp/parser.cx: (cp_parser_question_colon_clause):

s/x:// above

>         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
?

bool a;

int foo ()
{
  return a ?: 2;
}

will have true in the omitted operand always as well.

	Jakub
Manuel López-Ibáñez June 24, 2010, 12:52 p.m. UTC | #2 
On 24 June 2010 14:26, Andi Kleen <andi@firstfloor.org> wrote:
> On Thu, Jun 24, 2010 at 01:50:59PM +0200, Manuel López-Ibáñez wrote:
>> On 24 June 2010 13:24, Andi Kleen <andi@firstfloor.org> wrote:
>> > On Thu, Jun 03, 2010 at 11:18:18AM +0200, Andi Kleen wrote:
>> >>
>> >> Ping, can someone commit this please?
>> >
>> > *ping^2*
>>
>> If the patch is approved, please rediff against a recent revision,
>> send it with the changelog (or provide me with a link to the
>> changelog) and I will commit it for you.
>
> Here's a rediffed patch.
> -Andi
>
> gcc/
> 2010-05-31  Andi Kleen  <ak@linux.intel.com>
>        * 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.

c-family has its own Changelog.

>        * cp/parser.cx: (cp_parser_question_colon_clause):

extra x

> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/warn/warn-omitted-condop.C
> @@ -0,0 +1,58 @@
> +/* { dg-options "-Wparentheses" } */

This testcase has two times the same content (patch over existing file?)

> +++ b/gcc/testsuite/gcc.dg/warn-omitted-condop.c
> @@ -0,0 +1,58 @@
> +/* { dg-options "-Wparentheses" } */
> +

... and the C and C++ testcases are the same. So moving them to c-c++-common.


Committed revision 161318 with the above changes and the following
commit message:

2010-06-24  Andi Kleen  <ak@linux.intel.com>

        * c-parser.c (c_parser_conditional_expression):
        Call warn_for_omitted_condop.
        * doc/invoke.texi: Document omitted condop warning.
c-family/
        * c-common.c (warn_for_omitted_condop): New.
        * c-common.h (warn_for_omitted_condop): Add prototype.
testsuite/
        * c-c++-common/warn-omitted-condop.c: New.
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.
Paolo Bonzini June 24, 2010, 1:53 p.m. UTC | #3 
On 06/24/2010 02:37 PM, Jakub Jelinek wrote:
> 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
> ?
>
> bool a;
>
> int foo ()
> {
>    return a ?: 2;
> }
>
> will have true in the omitted operand always as well.

I think this is fine.  The text of the warning does not hint very much 
at why the code "smells", but the main idea was to warn for "a != 0 ?: b".

Paolo
diff mbox

Patch

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 %<true%>, "
+		"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" } */
+}