re PR c/32061 ((Wlogical-op) wording of warning of constant logicials need improvement)

2009-04-19  Manuel López-Ibáñez  <manu@gcc.gnu.org>

	PR c/32061
	PR c++/36954
	* doc/invoke.texi: Add -Wlogical-op to -Wextra.
	* common.opt (Wlogical-op): Move from here...
	* c.opt (Wlogical-op): ... to here.
	* c-typeck.c (parser_build_binary_op): Update call to
	warn_logical_operator.
	* c-opts.c (c_common_post_options): Enable warn_logical_op with
	extra_warnings.
	* c-common.c (warn_logical_op): Update.
	* c-common.h (warn_logical_op): Update declaration.
cp/
	* call.c (build_new_op): Save the original codes of operands
	before folding.
	
testsuite/
	* gcc.dg/pr32061.c: New.
	* gcc.dg/Wlogical-op-1.c: Update.
	* g++.dg/warn/Wlogical-op-1.C: Update.
	* g++.dg/warn/pr36954.C: New.

From-SVN: r146344

gcc/ChangeLog

+2009-04-19  Manuel López-Ibáñez  <manu@gcc.gnu.org>
+
+	PR c/32061
+	PR c++/36954
+	* doc/invoke.texi: Add -Wlogical-op to -Wextra.
+	* common.opt (Wlogical-op): Move from here...
+	* c.opt (Wlogical-op): ... to here.
+	* c-typeck.c (parser_build_binary_op): Update call to
+	warn_logical_operator.
+	* c-opts.c (c_common_post_options): Enable warn_logical_op with
+	extra_warnings.
+	* c-common.c (warn_logical_op): Update.
+	* c-common.h (warn_logical_op): Update declaration.
+
 2009-04-19  Eric Botcazou  <ebotcazou@adacore.com>
 
 	* tree.c (protected_set_expr_location): Fix formatting.

gcc/c-common.c

@@ -1712,38 +1712,43 @@ overflow_warning (tree value)
     }
 }
 
-
-/* Warn about use of a logical || / && operator being used in a
-   context where it is likely that the bitwise equivalent was intended
-   by the programmer. CODE is the TREE_CODE of the operator, ARG1
-   and ARG2 the arguments.  */
+/* Warn about uses of logical || / && operator in a context where it
+   is likely that the bitwise equivalent was intended by the
+   programmer.  We have seen an expression in which CODE is a binary
+   operator used to combine expressions OP_LEFT and OP_RIGHT, which
+   before folding had CODE_LEFT and CODE_RIGHT.  */
 
 void
-warn_logical_operator (enum tree_code code, tree arg1, tree
-    arg2)
-{
-  switch (code)
-    {
-      case TRUTH_ANDIF_EXPR:
-      case TRUTH_ORIF_EXPR:
-      case TRUTH_OR_EXPR:
-      case TRUTH_AND_EXPR:
-	if (!TREE_NO_WARNING (arg1)
-	    && INTEGRAL_TYPE_P (TREE_TYPE (arg1))
-	    && !CONSTANT_CLASS_P (arg1)
-	    && TREE_CODE (arg2) == INTEGER_CST
-	    && !integer_zerop (arg2))
-	  {
-	    warning (OPT_Wlogical_op,
-		     "logical %<%s%> with non-zero constant "
-		     "will always evaluate as true",
-		     ((code == TRUTH_ANDIF_EXPR)
-		      || (code == TRUTH_AND_EXPR)) ? "&&" : "||");
-	    TREE_NO_WARNING (arg1) = true;
-	  }
-	break;
-      default:
-	break;
+warn_logical_operator (location_t location, enum tree_code code,
+		       enum tree_code code_left, tree op_left, 
+		       enum tree_code ARG_UNUSED (code_right), tree op_right)
+{
+  if (code != TRUTH_ANDIF_EXPR
+      && code != TRUTH_AND_EXPR
+      && code != TRUTH_ORIF_EXPR
+      && code != TRUTH_OR_EXPR)
+    return;
+
+  /* Warn if &&/|| are being used in a context where it is
+     likely that the bitwise equivalent was intended by the
+     programmer. That is, an expression such as op && MASK
+     where op should not be any boolean expression, nor a
+     constant, and mask seems to be a non-boolean integer constant.  */
+  if (!truth_value_p (code_left)
+      && INTEGRAL_TYPE_P (TREE_TYPE (op_left))
+      && !CONSTANT_CLASS_P (op_left)
+      && !TREE_NO_WARNING (op_left)
+      && TREE_CODE (op_right) == INTEGER_CST
+      && !integer_zerop (op_right)
+      && !integer_onep (op_right))
+    {
+      if (code == TRUTH_ORIF_EXPR || code == TRUTH_OR_EXPR)
+	warning_at (location, OPT_Wlogical_op, "logical %<or%>"
+		    " applied to non-boolean constant");
+      else
+	warning_at (location, OPT_Wlogical_op, "logical %<and%>"
+		    " applied to non-boolean constant");
+      TREE_NO_WARNING (op_left) = true;
     }
 }
 

gcc/c-common.h

@@ -801,7 +801,8 @@ extern bool strict_aliasing_warning (tree, tree, tree);
 extern void warnings_for_convert_and_check (tree, tree, tree);
 extern tree convert_and_check (tree, tree);
 extern void overflow_warning (tree);
-extern void warn_logical_operator (enum tree_code, tree, tree);
+extern void warn_logical_operator (location_t, enum tree_code,
+				   enum tree_code, tree, enum tree_code, tree);
 extern void check_main_parameter_types (tree decl);
 extern bool c_determine_visibility (tree);
 extern bool same_scalar_type_ignoring_signedness (tree, tree);

gcc/c-opts.c

@@ -1080,6 +1080,8 @@ c_common_post_options (const char **pfilename)
     warn_override_init = extra_warnings;
   if (warn_ignored_qualifiers == -1)
     warn_ignored_qualifiers = extra_warnings;
+  if (warn_logical_op == -1)
+    warn_logical_op = extra_warnings;
 
   /* -Wpointer-sign is disabled by default, but it is enabled if any
      of -Wall or -pedantic are given.  */

gcc/c-typeck.c

@@ -2925,8 +2925,9 @@ parser_build_binary_op (location_t location, enum tree_code code,
   if (warn_parentheses)
     warn_about_parentheses (code, code1, arg1.value, code2, arg2.value);
 
-  if (TREE_CODE_CLASS (code1) != tcc_comparison)
-    warn_logical_operator (code, arg1.value, arg2.value);
+  if (warn_logical_op)
+    warn_logical_operator (input_location, code,
+			   code1, arg1.value, code2, arg2.value);
 
   /* Warn about comparisons against string literals, with the exception
      of testing for equality or inequality of a string literal with NULL.  */

gcc/c.opt

@@ -284,6 +284,10 @@ Winvalid-pch
 C ObjC C++ ObjC++ Warning
 Warn about PCH files that are found but not used
 
+Wlogical-op
+C ObjC C++ ObjC++ Var(warn_logical_op) Init(-1) Warning 
+Warn when a logical operator is suspiciously always evaluating to true or false
+
 Wlong-long
 C ObjC C++ ObjC++ Var(warn_long_long) Init(1) Warning
 Do not warn about using \"long long\" when -pedantic

gcc/common.opt

@@ -128,10 +128,6 @@ Wlarger-than=
 Common RejectNegative Joined UInteger Warning
 -Wlarger-than=<number>	Warn if an object is larger than <number> bytes
 
-Wlogical-op
-Common Warning Var(warn_logical_op)
-Warn when a logical operator is suspicously always evaluating to true or false
-
 Wunsafe-loop-optimizations
 Common Var(warn_unsafe_loop_optimizations) Warning
 Warn if the loop cannot be optimized due to nontrivial assumptions.

gcc/cp/ChangeLog

+2009-04-19  Manuel López-Ibáñez  <manu@gcc.gnu.org>
+
+	PR c/32061
+	PR c++/36954
+	* call.c (build_new_op): Save the original codes of operands
+	before folding.
+
 2009-04-18  Kazu Hirata  <kazu@codesourcery.com>
 
 	* cp-tree.h: Remove the prototype for insert_block.

gcc/cp/call.c

@@ -3897,11 +3897,12 @@ build_new_op (enum tree_code code, int flags, tree arg1, tree arg2, tree arg3,
   tree result = NULL_TREE;
   bool result_valid_p = false;
   enum tree_code code2 = NOP_EXPR;
+  enum tree_code code_orig_arg1 = ERROR_MARK;
+  enum tree_code code_orig_arg2 = ERROR_MARK;
   conversion *conv;
   void *p;
   bool strict_p;
   bool any_viable_p;
-  bool expl_eq_arg1 = false;
 
   if (error_operand_p (arg1)
       || error_operand_p (arg2)
@@ -3935,8 +3936,10 @@ build_new_op (enum tree_code code, int flags, tree arg1, tree arg2, tree arg3,
     case TRUTH_ANDIF_EXPR:
     case TRUTH_AND_EXPR:
     case TRUTH_OR_EXPR:
-      if (COMPARISON_CLASS_P (arg1))
-	expl_eq_arg1 = true;
+      /* These are saved for the sake of warn_logical_operator.  */
+      code_orig_arg1 = TREE_CODE (arg1);
+      code_orig_arg2 = TREE_CODE (arg2);
+
     default:
       break;
     }
@@ -4140,8 +4143,16 @@ build_new_op (enum tree_code code, int flags, tree arg1, tree arg2, tree arg3,
 	  if (conv->kind == ck_ref_bind)
 	    conv = conv->u.next;
 	  arg1 = convert_like (conv, arg1, complain);
+
 	  if (arg2)
 	    {
+	      /* We need to call warn_logical_operator before
+		 converting arg2 to a boolean_type.  */
+	      if (complain & tf_warning)
+		warn_logical_operator (input_location, code,
+				       code_orig_arg1, arg1,
+				       code_orig_arg2, arg2);
+
 	      conv = cand->convs[1];
 	      if (conv->kind == ck_ref_bind)
 		conv = conv->u.next;
@@ -4155,12 +4166,6 @@ build_new_op (enum tree_code code, int flags, tree arg1, tree arg2, tree arg3,
 	      arg3 = convert_like (conv, arg3, complain);
 	    }
 
-	  if (!expl_eq_arg1) 
-	    {
-	      if (complain & tf_warning)
-		warn_logical_operator (code, arg1, arg2);
-	      expl_eq_arg1 = true;
-	    }
 	}
     }
 
@@ -4185,8 +4190,9 @@ build_new_op (enum tree_code code, int flags, tree arg1, tree arg2, tree arg3,
     case TRUTH_ORIF_EXPR:
     case TRUTH_AND_EXPR:
     case TRUTH_OR_EXPR:
-      if (!expl_eq_arg1)
-	warn_logical_operator (code, arg1, arg2);
+      warn_logical_operator (input_location, code,
+			     code_orig_arg1, arg1, code_orig_arg2, arg2);
+      /* Fall through.  */
     case PLUS_EXPR:
     case MINUS_EXPR:
     case MULT_EXPR:

gcc/doc/invoke.texi

@@ -2804,6 +2804,7 @@ name is still supported, but the newer name is more descriptive.)
 @gccoptlist{-Wclobbered  @gol
 -Wempty-body  @gol
 -Wignored-qualifiers @gol
+-Wlogical-op @gol
 -Wmissing-field-initializers  @gol
 -Wmissing-parameter-type @r{(C only)}  @gol
 -Wold-style-declaration @r{(C only)}  @gol
@@ -3790,7 +3791,8 @@ programmer intended to use @code{strcmp}.  This warning is enabled by
 @opindex Wno-logical-op
 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.
+bit-wise operator is likely to be expected.  This warning is enabled by
+@option{-Wextra}.
 
 @item -Waggregate-return
 @opindex Waggregate-return

gcc/testsuite/ChangeLog

+2009-04-19  Manuel López-Ibáñez  <manu@gcc.gnu.org>
+
+	PR c/32061
+	PR c++/36954
+	* gcc.dg/pr32061.c: New.
+	* gcc.dg/Wlogical-op-1.c: Update.
+	* g++.dg/warn/Wlogical-op-1.C: Update.
+	* g++.dg/warn/pr36954.C: New.
+
 2009-04-18  Joseph Myers  <joseph@codesourcery.com>
 
 	PR c/27676

gcc/testsuite/g++.dg/warn/Wlogical-op-1.C

@@ -28,20 +28,39 @@ extern testenum testa();
 
 void foo()
 {
-    if ( f && b2 )            // { dg-warning "always evaluate as" }
+    if ( f && b2 )            // { dg-warning "logical" }
           do_something(1);
-    if ( c && b2 )            // { dg-warning "always evaluate as" }
+    if ( c && b2 )            // { dg-warning "logical" }
           do_something(2);
 
-    if ( b2 && c == a )       // { dg-bogus "always evaluate as" }
+    if ( b2 && c == a )       // { dg-bogus "logical" }
           do_something(101);
     if ( 1 && c )
-          do_something(102);  // { dg-bogus "always evaluate as" }
-    if ( t2 && b2 )           // { dg-bogus "always evaluate as" }
+          do_something(102);  // { dg-bogus "logical" }
+    if ( t2 && b2 )           // { dg-bogus "logical" }
           do_something(103);
-    if ( true && c == a )     // { dg-bogus "always evaluate as" }
+    if ( true && c == a )     // { dg-bogus "logical" }
           do_something(104);
-    if ( b2 && true )         // { dg-bogus "always evaluate as" }
+    if ( b2 && true )         // { dg-bogus "logical" }
           do_something(105);
 }
 
+
+void bar()
+{
+    if ( f || b2 )            // { dg-warning "logical" }
+          do_something(1);
+    if ( c || b2 )            // { dg-warning "logical" }
+          do_something(2);
+
+    if ( b2 || c == a )       // { dg-bogus "logical" }
+          do_something(101);
+    if ( 1 || c )
+          do_something(102);  // { dg-bogus "logical" }
+    if ( t2 || b2 )           // { dg-bogus "logical" }
+          do_something(103);
+    if ( true || c == a )     // { dg-bogus "logical" }
+          do_something(104);
+    if ( b2 || true )         // { dg-bogus "logical" }
+          do_something(105);
+}

gcc/testsuite/g++.dg/warn/pr36954.C

+// PR c++/36954
+// { dg-do compile }
+// { dg-options "-Wlogical-op -Wextra -Wall" }
+
+template<class C> void Test()
+{
+  if ((1 == 2) || (true)) { 
+  }
+
+  if ((1 == 2) || (!false)) {
+  }
+
+  if (false || true) {
+  }
+}
+
+
+
+int main() {
+  if ((1 == 2) || (true)) {
+  }
+}
+

gcc/testsuite/gcc.dg/Wlogical-op-1.c

@@ -14,34 +14,64 @@ extern int testa();
 
 void foo()
 {
-    if ( testa() && b )     /* { dg-warning "always evaluate as" } */
+    if ( testa() && b )     /* { dg-warning "logical" } */
          (void)testa();
 
-    if ( c && b )           /* { dg-warning "always evaluate as" } */
+    if ( c && b )           /* { dg-warning "logical" } */
 	(void)testa();
 
-    if ( c && 0x42 )        /* { dg-warning "always evaluate as" } */
+    if ( c && 0x42 )        /* { dg-warning "logical" } */
 	(void)testa();
 
-    if ( c && 0x42 )        /* { dg-warning "always evaluate as" } */
+    if ( c && 0x80 >>6)     /* { dg-warning "logical" } */
+	(void)testa();
+
+
+    if ( b && c == a )      /* { dg-bogus "logical" } */
+          (void)testa();
+
+    if ( 1 && c )           /* { dg-bogus "logical" } */
+         (void)testa();
+
+    if ( t2 && b )          /* { dg-bogus "logical" } */
+          (void)testa();
+
+    if ( 0 && c == a )      /* { dg-bogus "logical" } */
+          (void)testa();
+
+    if ( b && 1 )           /* { dg-bogus "logical" } */
+          (void)testa();
+}
+
+
+void bar()
+{
+    if ( testa() || b )     /* { dg-warning "logical" } */
+         (void)testa();
+
+    if ( c || b )           /* { dg-warning "logical" } */
+	(void)testa();
+
+    if ( c || 0x42 )        /* { dg-warning "logical" } */
 	(void) testa();
 
-    if ( c && 0x80 >>6)     /* { dg-warning "always evaluate as" } */
+    if ( c || 0x80 >>6)     /* { dg-warning "logical" } */
 	(void)testa();
 
 
-    if ( b && c == a )      /* { dg-bogus "always evaluate as" } */
+    if ( b || c == a )      /* { dg-bogus "logical" } */
           (void)testa();
 
-    if ( 1 && c )           /* { dg-bogus "always evaluate as" } */
+    if ( 1 || c )           /* { dg-bogus "logical" } */
          (void)testa();
 
-    if ( t2 && b )          /* { dg-bogus "always evaluate as" } */
+    if ( t2 || b )          /* { dg-bogus "logical" } */
           (void)testa();
 
-    if ( 0 && c == a )      /* { dg-bogus "always evaluate as" } */
+    if ( 0 || c == a )      /* { dg-bogus "logical" } */
           (void)testa();
 
-    if ( b && 1 )           /* { dg-bogus "always evaluate as" } */
+    if ( b || 1 )           /* { dg-bogus "logical" } */
           (void)testa();
 }
+

gcc/testsuite/gcc.dg/pr32061.c

+/* PR c/32061
+   { dg-do compile } 
+   { dg-options "-Wlogical-op -Wall -Wextra" }
+*/
+#define FORCE   1
+#define FLAG    1
+int func (int resp, int flags)
+{
+  return (resp && (FORCE || (FLAG & flags)));
+}