[PATCH] Warn about bogus conditional operators

[Manuel López-Ibáñez]

2009/1/16 Andi Kleen <andi@firstfloor.org>:
>
> Some time ago I fell into the trap of typing x > y ? : y which
> is equivalent to x > y ? 1 : y
> was obviously not what I meant, but instead wanted to pass out x.
> I don't think it's ever very useful to omit the second operand
> if the condition is a boolean, so let's add a warning for it.
>
> -Andi
>
> 2008-12-11  Andi Kleen  <ak@linux.intel.com>
>
>        * doc/invoke.texi: Document -Wno-omitted-conditional-op
>        * gcc.dg/warn-omitted-condop.c: Add
>        * g++.dg/warn-omitted-condop.C: Add
>        * cp/parser.c: (cp_parser_question_colon_clause):
>        Call warn_for_omitted_condop.
>        * 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.
>        * common.opt: Add Womitted-conditional-op.
>
> Index: gcc/doc/invoke.texi
> ===================================================================
> --- gcc/doc/invoke.texi (revision 143064)
> +++ gcc/doc/invoke.texi (working copy)
> @@ -263,7 +263,7 @@
>  -Wunused  -Wunused-function  -Wunused-label  -Wunused-parameter @gol
>  -Wunused-value  -Wunused-variable @gol
>  -Wvariadic-macros -Wvla @gol
> --Wvolatile-register-var  -Wwrite-strings}
> +-Wvolatile-register-var  -Wwrite-strings -Womitted-conditional-op}
>
>  @item C and Objective-C-only Warning Options
>  @gccoptlist{-Wbad-function-cast  -Wmissing-declarations @gol
> @@ -4146,6 +4146,15 @@
>  If any of @var{sym} is called, GCC will issue a warning. This can be useful
>  in enforcing coding conventions that ban calls to certain functions, for
>  example, @code{alloca}, @code{malloc}, etc.
> +
> +@item -Wno-omitted-conditional-op
> +@opindex Wno-omitted-conditional-op
> +warn for dangerous uses of the
> +?: with omitted middle operand GNU extension. When the condition
> +in the ?: operator is a computed boolean the omitted value will
> +be always 1. Often the user expects it to be a value computed
> +inside the conditional expression instead. Gcc by default warns
> +for this, but this option disables it.
>  @end table
>
>  @node Debugging Options
> Index: gcc/testsuite/gcc.dg/warn-omitted-condop.c
> ===================================================================
> --- gcc/testsuite/gcc.dg/warn-omitted-condop.c  (revision 0)
> +++ gcc/testsuite/gcc.dg/warn-omitted-condop.c  (revision 0)
> @@ -0,0 +1,29 @@
> +/* { dg-options "-Womitted-conditional-op" } */
> +
> +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 "Omitting middle operand" } */
> +  T(>); /* { dg-warning "Omitting middle operand" } */
> +  T(<=); /* { dg-warning "Omitting middle operand" } */
> +  T(>=); /* { dg-warning "Omitting middle operand" } */
> +  T(==); /* { dg-warning "Omitting middle operand" } */
> +  T(!=); /* { dg-warning "Omitting middle operand" } */
> +  T(||); /* { dg-warning "Omitting middle operand" } */
> +  T(&&); /* { dg-warning "Omitting middle operand" } */
> +  f2 (!x ? : 1);  /* { dg-warning "Omitting middle operand" } */
> +  T2(<); /* { dg-bogus "Omitting middle operand" } */
> +  T2(>); /* { dg-bogus "Omitting middle operand" } */
> +  T2(==); /* { dg-bogus "Omitting middle operand" } */
> +  T2(||); /* { dg-bogus "Omitting middle operand" } */
> +  T2(&&); /* { dg-bogus "Omitting middle operand" } */
> +  T(+); /* { dg-bogus "Omitting middle operand" } */
> +  T(-); /* { dg-bogus "Omitting middle operand" } */
> +  T(*); /* { dg-bogus "Omitting middle operand" } */
> +  T(/); /* { dg-bogus "Omitting middle operand" } */
> +  T(^); /* { dg-bogus "Omitting middle operand" } */
> +}
> Index: gcc/testsuite/g++.dg/warn/warn-omitted-condop.C
> ===================================================================
> --- gcc/testsuite/g++.dg/warn/warn-omitted-condop.C     (revision 0)
> +++ gcc/testsuite/g++.dg/warn/warn-omitted-condop.C     (revision 0)
> @@ -0,0 +1,29 @@
> +/* { dg-options "-Womitted-conditional-op" } */
> +
> +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 "Omitting middle operand" } */
> +  T(>); /* { dg-warning "Omitting middle operand" } */
> +  T(<=); /* { dg-warning "Omitting middle operand" } */
> +  T(>=); /* { dg-warning "Omitting middle operand" } */
> +  T(==); /* { dg-warning "Omitting middle operand" } */
> +  T(!=); /* { dg-warning "Omitting middle operand" } */
> +  T(||); /* { dg-warning "Omitting middle operand" } */
> +  T(&&); /* { dg-warning "Omitting middle operand" } */
> +  f2 (!x ? : 1);  /* { dg-warning "Omitting middle operand" } */
> +  T2(<); /* { dg-bogus "Omitting middle operand" } */
> +  T2(>); /* { dg-bogus "Omitting middle operand" } */
> +  T2(==); /* { dg-bogus "Omitting middle operand" } */
> +  T2(||); /* { dg-bogus "Omitting middle operand" } */
> +  T2(&&); /* { dg-bogus "Omitting middle operand" } */
> +  T(+); /* { dg-bogus "Omitting middle operand" } */
> +  T(-); /* { dg-bogus "Omitting middle operand" } */
> +  T(*); /* { dg-bogus "Omitting middle operand" } */
> +  T(/); /* { dg-bogus "Omitting middle operand" } */
> +  T(^); /* { dg-bogus "Omitting middle operand" } */
> +}
> Index: gcc/cp/parser.c
> ===================================================================
> --- gcc/cp/parser.c     (revision 143064)
> +++ gcc/cp/parser.c     (working copy)
> @@ -6336,11 +6336,16 @@
>   cp_lexer_consume_token (parser->lexer);
>   if (cp_parser_allow_gnu_extensions_p (parser)
>       && cp_lexer_next_token_is (parser->lexer, CPP_COLON))
> +  {
> +    warn_for_omitted_condop (logical_or_expr);
>     /* Implicit true clause.  */
>     expr = NULL_TREE;
> +  }
>   else
> +  {
>     /* Parse the expression.  */
>     expr = cp_parser_expression (parser, /*cast_p=*/false);
> +  }
>
>   /* The next token should be a `:'.  */
>   cp_parser_require (parser, CPP_COLON, "%<:%>");
> Index: gcc/common.opt
> ===================================================================
> --- gcc/common.opt      (revision 143064)
> +++ gcc/common.opt      (working copy)
> @@ -237,6 +237,10 @@
>  Common RejectNegative Var(warn_coverage_mismatch) Warning
>  Warn instead of error in case profiles in -fprofile-use do not match
>
> +Womitted-conditional-op
> +Common Var(warn_omitted_condop) Init(1)
> +Warn for omitted middle operands in ?: with unexpected meaning
> +
>  aux-info
>  Common Separate
>  -aux-info <file>       Emit declaration information into <file>
> Index: gcc/c-common.c
> ===================================================================
> --- gcc/c-common.c      (revision 143064)
> +++ gcc/c-common.c      (working copy)
> @@ -8362,4 +8362,26 @@
>     }
>  }
>
> +/* Warn for A ?: C expressions (with B omitted) where A is a computed
> +   boolean expression. */
> +
> +void
> +warn_for_omitted_condop (tree cond)
> +{
> +  enum tree_code code = TREE_CODE (cond);
> +
> +  if (!warn_omitted_condop)
> +    return;
> +  if (TREE_CODE_CLASS (code) == tcc_comparison ||
> +      code == TRUTH_ANDIF_EXPR ||
> +      code == TRUTH_ORIF_EXPR ||
> +      code == TRUTH_ANDIF_EXPR ||
> +      code == TRUTH_NOT_EXPR)
> +    {
> +      warning (OPT_Womitted_conditional_op,
> +              "Omitting middle operand in ?: with computed "
> +              "boolean conditional has unexpected meaning");
> +    }
> +}

This warning is not very informative. If the warning was fair, you do
not explain what is wrong (that 'true" will be always used as the
middle operand). If the warning is unfair, then the meaning is not
unexpected and you are completely dumbfounding the user.

I'll propose something along the lines of:

> +      warning (OPT_Wparentheses,
> +              "the omitted middle operand in ?: will always be %<true%>, suggest explicit middle operand");

I also think that having a new option for this warning is too much.
This can be included in Wparentheses or another option.

Cheers,

Manuel.