[PATCH] Add -Womitted-conditional-op warning

[Andi Kleen <ak at suse dot de>]

Since it seems to be currently envogue to add new warnings: I had this one
lying around for quite some time. It checks for a (relatively obscure) mistake 
in using a GCC extension that I once made. Gcc allows omitting the middle
operand in ?:. Now when one writes e.g.  

foo() != NULL ?: bar

then the result is typically unexpected because the omitted value will be always
1. But at least I foolishly expected it to be the return value of foo(), because
reusing the value from the guarding expression is normally the reason for using
?:.

I implemented a warning for this case. I guess it's relatively obscure, but will
not hurt. The warning is enabled by default, but can be disabled with
-Wno-omitted-conditional-op.

Passed C/C++ bootstrap and a test suite run on x86_64-linux.

-Andi

2007-02-02  Andi Kleen  <ak@suse.de>

	* c-common.c, c-common.h (warn_for_omitted_condop): 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.	
	* common.opt: Add -Womitted-conditional-op.
	* doc/invoke.text: Document -Wno-omitted-conditional-op.
	* testsuite/gcc.dg/warn-omitted-condop.c: Add.
	* testsuite/g++.dg/warn-omitted-condop.C: Add.

Index: gcc/doc/invoke.texi
===================================================================
--- gcc/doc/invoke.texi	(revision 121493)
+++ gcc/doc/invoke.texi	(working copy)
@@ -253,7 +253,7 @@
 -Wunknown-pragmas  -Wno-pragmas -Wunreachable-code @gol
 -Wunused  -Wunused-function  -Wunused-label  -Wunused-parameter @gol
 -Wunused-value  -Wunused-variable  -Wvariadic-macros @gol
--Wvolatile-register-var  -Wwrite-strings}
+-Wvolatile-register-var  -Wwrite-strings -Wno-omitted-conditional-op}
 
 @item C-only Warning Options
 @gccoptlist{-Wbad-function-cast  -Wmissing-declarations @gol
@@ -3613,6 +3613,15 @@
 
 This option is implied by @option{-pedantic}, and can be disabled with
 @option{-Wno-overlength-strings}.
+
+@item -Wno-omitted-conditional-op
+@opindex Wno-omitted-conditional-op
+Don't 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/cp/parser.c
===================================================================
--- gcc/cp/parser.c	(revision 121493)
+++ gcc/cp/parser.c	(working copy)
@@ -5856,11 +5856,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 121493)
+++ gcc/common.opt	(working copy)
@@ -201,6 +201,10 @@
 Common RejectNegative Var(warn_coverage_mismatch)
 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 121493)
+++ gcc/c-common.c	(working copy)
@@ -6783,4 +6828,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");
+    }    
+} 
+
 #include "gt-c-common.h"
Index: gcc/c-common.h
===================================================================
--- gcc/c-common.h	(revision 121493)
+++ gcc/c-common.h	(working copy)
@@ -870,6 +870,7 @@
 				    enum tree_code);
 extern void warn_for_unused_label (tree label);
 
+extern void warn_for_omitted_condop (tree cond);
 
 /* In c-gimplify.c  */
 extern void c_genericize (tree);
Index: gcc/c-parser.c
===================================================================
--- gcc/c-parser.c	(revision 121493)
+++ gcc/c-parser.c	(working copy)
@@ -4388,6 +4388,7 @@
     {
       if (pedantic)
 	pedwarn ("ISO C forbids omitting the middle term of a ?: expression");
+      warn_for_omitted_condop (cond.value);
       /* Make sure first operand is calculated only once.  */
       exp1.value = save_expr (default_conversion (cond.value));
       cond.value = c_objc_common_truthvalue_conversion (exp1.value);
--- /dev/null	2006-11-25 12:43:29.000000000 +0100
+++ gcc/testsuite/gcc.dg/warn-omitted-condop.c	2007-02-02 15:46:10.000000000 +0100
@@ -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" } */
+}
--- /dev/null	2006-11-25 12:43:29.000000000 +0100
+++ gcc/testsuite/g++.dg/warn/warn-omitted-condop.C	2007-02-02 17:11:27.000000000 +0100
@@ -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" } */
+}