This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.

Index Nav:	[Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav:	[Date Prev] [Date Next]	[Thread Prev] [Thread Next]
Other format:	[Raw text]

Re: [PATCH] Implement warning for wrong use of ||/&&

From: "Manuel López-Ibáñez" <lopezibanez at gmail dot com>
To: "Dirk Mueller" <dmueller at suse dot de>
Cc: gcc-patches at gcc dot gnu dot org
Date: Mon, 12 Feb 2007 23:20:50 +0000
Subject: Re: [PATCH] Implement warning for wrong use of ||/&&
References: <200702130009.06245.dmueller@suse.de>

No, please, don't do this. Walways-true warnings have nothing to do
with this (the name is wrong wrong wrong, this is the same mistake
that was done with Wconversion, but worse because this time the
description of Walways-true is also wrong, so please let's not repeat
it again).

And I cannot understand how you were able to bootstrapped this if it
emits warnings for GCC code.

Regards,

Manuel.

PS: I think there are already warnings for wrong usage of || and &&
somewhere. If you don't want to bother, I would like the patch to be
hold until I can look them up.

On 12/02/07, Dirk Mueller <dmueller@suse.de> wrote:

Hi,

the patch below implements a warning for apparently wrong usage of "||"
and "&&" in contexts where likely a "|" or "&" was intended by the programmer.
This seems very low false-positive to me, and it has found dozens of real
faults in real world code (actually all code I tried it on, including gcc
itself). I was actually trying to submit patches for those all by myself to
get all the  honor, but on the other hand I'm too lazy for that so I'd rather
like to get the warning into gcc so that everyone can fix their code
instead ;)

I've spent several hours trying to come up for a good name for this warning,
but couldn't. so I've plumbed it into -Walways-true. Matches perfectly IMHO.

bootstrapped, regtested against i686-suse-linux, no regressions. Ok?


Thanks,
Dirk

2007-02-13 Dirk Mueller <dmueller@suse.de>

        * c-typeck.c (parser_build_binary_op): Warn about expressions
        of &&/|| and non-zero constants.

        * cp/call.c (build_new_op): Warn about expressions
        of &&/|| and non-zero constants.

        * gcc.dg/Walways-true-3.c: New testcase.
        * g++.dg/warn/Walways-true-3.C: New testcase.

--- c-typeck.c  (revision 121595)
+++ c-typeck.c  (working copy)
@@ -2634,6 +2634,33 @@ parser_build_binary_op (enum tree_code c
   if (warn_parentheses)
     warn_about_parentheses (code, code1, code2);

+  switch (code)
+    {
+      case TRUTH_ANDIF_EXPR:
+      case TRUTH_ORIF_EXPR:
+      case TRUTH_OR_EXPR:
+      case TRUTH_AND_EXPR:
+       if (!TREE_NO_WARNING (arg1.value)
+            && INTEGRAL_TYPE_P (TREE_TYPE (arg1.value))
+           && !CONSTANT_CLASS_P (arg1.value)
+           && TREE_CODE_CLASS (code1) != tcc_comparison
+           && TREE_CODE (arg2.value) == INTEGER_CST
+           && !integer_zerop (arg2.value)
+           && !integer_onep (arg2.value))
+          {
+           warning (OPT_Walways_true,
+                    "logical %<%s%> with non-zero constant "
+                    "will always evaluate as true",
+                    ((code == TRUTH_ANDIF_EXPR)
+                     || (code == TRUTH_AND_EXPR)) ? "&&" : "||");
+            TREE_NO_WARNING (arg1.value) = 1;
+          }
+
+       break;
+      default:
+       break;
+    }
+
   /* Warn about comparisons against string literals, with the exception
      of testing for equality or inequality of a string literal with NULL.  */
   if (code == EQ_EXPR || code == NE_EXPR)
--- cp/call.c   (revision 121595)
+++ cp/call.c   (working copy)
@@ -3665,6 +3680,8 @@ build_new_op (enum tree_code code, int f
   void *p;
   bool strict_p;
   bool any_viable_p;
+  bool expl_eq_arg1 = false;
+  bool expl_eq_arg2 = false;

   if (error_operand_p (arg1)
       || error_operand_p (arg2)
@@ -3694,6 +3711,14 @@ build_new_op (enum tree_code code, int f
     case CALL_EXPR:
       return build_object_call (arg1, arg2);

+    case TRUTH_ORIF_EXPR:
+    case TRUTH_ANDIF_EXPR:
+    case TRUTH_AND_EXPR:
+    case TRUTH_OR_EXPR:
+      if (COMPARISON_CLASS_P (arg1))
+       expl_eq_arg1 = true;
+      if (COMPARISON_CLASS_P (arg2))
+       expl_eq_arg2 = true;
     default:
       break;
     }
@@ -3901,6 +3926,28 @@ build_new_op (enum tree_code code, int f
                conv = conv->u.next;
              arg3 = convert_like (conv, arg3);
            }
+
+         switch (code)
+           {
+           case TRUTH_ANDIF_EXPR:
+           case TRUTH_ORIF_EXPR:
+            case TRUTH_AND_EXPR:
+            case TRUTH_OR_EXPR:
+             if (INTEGRAL_TYPE_P (TREE_TYPE (arg1))
+                   && !CONSTANT_CLASS_P (arg1)
+                   && !expl_eq_arg1
+                   && TREE_CODE (arg2) == INTEGER_CST
+                   && !integer_zerop (arg2))
+               {
+                 warning (OPT_Walways_true, "logical %<%s%> with non-zero constant "
+                          "will always evaluate as true",
+                          ((code == TRUTH_ANDIF_EXPR)
+                           || (code == TRUTH_AND_EXPR)) ? "&&" : "||");
+                 expl_eq_arg1 = expl_eq_arg2 = true;
+               }
+           default:
+             break;
+           }
        }
     }

@@ -3921,6 +3968,18 @@ build_new_op (enum tree_code code, int f
     case INDIRECT_REF:
       return build_indirect_ref (arg1, "unary *");

+    case TRUTH_ANDIF_EXPR:
+    case TRUTH_ORIF_EXPR:
+      if (INTEGRAL_TYPE_P (TREE_TYPE (arg1))
+          && !CONSTANT_CLASS_P (arg1)
+          && !expl_eq_arg1
+          && TREE_CODE (arg2) == INTEGER_CST
+          && !integer_zerop (arg2)
+          && !integer_onep (arg2))
+       warning (OPT_Walways_true, "logical %<%s%> with non-zero constant "
+                "will always evalue as true",
+                ((code == TRUTH_ANDIF_EXPR)
+                 || (code == TRUTH_AND_EXPR)) ? "&&" : "||");
     case PLUS_EXPR:
     case MINUS_EXPR:
     case MULT_EXPR:
@@ -3939,8 +3998,6 @@ build_new_op (enum tree_code code, int f
     case BIT_AND_EXPR:
     case BIT_IOR_EXPR:
     case BIT_XOR_EXPR:
-    case TRUTH_ANDIF_EXPR:
-    case TRUTH_ORIF_EXPR:
       return cp_build_binary_op (code, arg1, arg2);

     case UNARY_PLUS_EXPR:
Index: testsuite/gcc.dg/Walways-true-3.c
===================================================================
--- testsuite/gcc.dg/Walways-true-3.c   (revision 0)
+++ testsuite/gcc.dg/Walways-true-3.c   (revision 0)
@@ -0,0 +1,47 @@
+/*
+   { dg-do compile}
+   { dg-options "-Walways-true" }
+*/
+
+enum { a, ba, b };
+
+enum testenum { t1, t2};
+
+extern int c;
+extern char bool_a, bool_b;
+
+extern int testa();
+
+void foo()
+{
+    if ( testa() && b )     /* { dg-warning "always evaluate as" } */
+         (void)testa();
+
+    if ( c && b )           /* { dg-warning "always evaluate as" } */
+       (void)testa();
+
+    if ( c && 0x42 )        /* { dg-warning "always evaluate as" } */
+       (void)testa();
+
+    if ( c && 0x42 )        /* { dg-warning "always evaluate as" } */
+       (void) testa();
+
+    if ( c && 0x80 >>6)     /* { dg-warning "always evaluate as" } */
+       (void)testa();
+
+
+    if ( b && c == a )      /* { dg-bogus "always evaluate as" } */
+          (void)testa();
+
+    if ( 1 && c )           /* { dg-warning "always evaluate as" } */
+         (void)testa();
+
+    if ( t2 && b )          /* { dg-warning "always evaluate as" } */
+          (void)testa();
+
+    if ( 0 && c == a )      /* { dg-warning "always evaluate as" } */
+          (void)testa();
+
+    if ( b && 1 )           /* { dg-warning "always evaluate as" } */
+          (void)testa();
+}
Index: testsuite/g++.dg/warn/Walways-true-3.C
===================================================================
--- testsuite/g++.dg/warn/Walways-true-3.C      (revision 0)
+++ testsuite/g++.dg/warn/Walways-true-3.C      (revision 0)
@@ -0,0 +1,46 @@
+// { dg-do compile}
+// { dg-options "-Walways-true" }
+
+enum { a, b };
+
+enum testenum { t1, t2};
+
+extern int c;
+extern bool bool_a, bool_b;
+
+template<typename Enum>
+class QFlags
+{
+public:
+    typedef void **Zero;
+    int i;
+    inline QFlags(Enum f) : i(f) {}
+
+    inline operator int() const
+    { return i;}
+
+};
+
+QFlags<testenum> f(t2);
+extern void do_something(int);
+
+extern testenum testa();
+
+void foo()
+{
+    if ( f && b )             // { dg-warning "always evaluate as" }
+          do_something(1);
+    if ( c && b )             // { dg-warning "always evaluate as" }
+          do_something(2);
+
+    if ( b && c == a )        // { dg-bogus "always evaluate as" }
+          do_something(101);
+    if ( 1 && c )
+          do_something(102);  // { dg-bogus "always evaluate as" }
+    if ( t2 && b )            // { dg-bogus "always evaluate as" }
+          do_something(103);
+    if ( true && c == a )     // { dg-bogus "always evaluate as" }
+          do_something(104);
+    if ( b && true )          // { dg-bogus "always evaluate as" }
+          do_something(105);
+}

Follow-Ups:
- Re: [PATCH] Implement warning for wrong use of ||/&&
  - From: Dirk Mueller

References:
- [PATCH] Implement warning for wrong use of ||/&&
  - From: Dirk Mueller

Index Nav:	[Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav:	[Date Prev] [Date Next]	[Thread Prev] [Thread Next]