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

diff --git a/gcc/ChangeLog b/gcc/ChangeLog
index 066f16c..09f17e5 100644 (file)
--- a/gcc/ChangeLog
+++ b/gcc/ChangeLog
@@ -1,3 +1,17 @@
+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.

diff --git a/gcc/c-common.c b/gcc/c-common.c
index 77f7ebc..735c8e0 100644 (file)
--- a/gcc/c-common.c
+++ b/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;
     }
 }
 

diff --git a/gcc/c-common.h b/gcc/c-common.h
index 061cbe3..723f8b5 100644 (file)
--- a/gcc/c-common.h
+++ b/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);

diff --git a/gcc/c-opts.c b/gcc/c-opts.c
index f619522..ece4d7f 100644 (file)
--- a/gcc/c-opts.c
+++ b/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.  */

diff --git a/gcc/c-typeck.c b/gcc/c-typeck.c
index 77eafbf..821f4ce 100644 (file)
--- a/gcc/c-typeck.c
+++ b/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.  */

diff --git a/gcc/c.opt b/gcc/c.opt
index 2a1b740..b3e7dd7 100644 (file)
--- a/gcc/c.opt
+++ b/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

diff --git a/gcc/common.opt b/gcc/common.opt
index 20dd071..c0f6b9e 100644 (file)
--- a/gcc/common.opt
+++ b/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.

diff --git a/gcc/cp/ChangeLog b/gcc/cp/ChangeLog
index bfa05e5..259aa8e 100644 (file)
--- a/gcc/cp/ChangeLog
+++ b/gcc/cp/ChangeLog
@@ -1,3 +1,10 @@
+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.

diff --git a/gcc/cp/call.c b/gcc/cp/call.c
index f6c24b6..feb3004 100644 (file)
--- a/gcc/cp/call.c
+++ b/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:

diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi
index 3c49a8d..e1dd0e0 100644 (file)
--- a/gcc/doc/invoke.texi
+++ b/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

diff --git a/gcc/testsuite/ChangeLog b/gcc/testsuite/ChangeLog
index c87827b..361d93d 100644 (file)
--- a/gcc/testsuite/ChangeLog
+++ b/gcc/testsuite/ChangeLog
@@ -1,3 +1,12 @@
+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

diff --git a/gcc/testsuite/g++.dg/warn/Wlogical-op-1.C b/gcc/testsuite/g++.dg/warn/Wlogical-op-1.C
index f67ab89..61d4a9d 100644 (file)
--- a/gcc/testsuite/g++.dg/warn/Wlogical-op-1.C
+++ b/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);
+}

diff --git a/gcc/testsuite/g++.dg/warn/pr36954.C b/gcc/testsuite/g++.dg/warn/pr36954.C
new file mode 100644 (file)
index 0000000..92cea2f
--- /dev/null
+++ b/gcc/testsuite/g++.dg/warn/pr36954.C
@@ -0,0 +1,23 @@
+// 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)) {
+  }
+}
+

diff --git a/gcc/testsuite/gcc.dg/Wlogical-op-1.c b/gcc/testsuite/gcc.dg/Wlogical-op-1.c
index d9687bf..2cbb980 100644 (file)
--- a/gcc/testsuite/gcc.dg/Wlogical-op-1.c
+++ b/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();
 }
+

diff --git a/gcc/testsuite/gcc.dg/pr32061.c b/gcc/testsuite/gcc.dg/pr32061.c
new file mode 100644 (file)
index 0000000..dc1a916
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/pr32061.c
@@ -0,0 +1,10 @@
+/* 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)));
+}