Re: PING: [PATCH] correct "comparisons do not have their mathematical meaning" warning

[Dirk Mueller <dmueller at suse dot de>]

On Monday, 8. January 2007 06:28, Ian Lance Taylor wrote:

> This patch as it stands does not look right.  In the second block it
> checks TREE_CODE_CLASS (code) twice

Whoops :) Fixed. 

> and does not check TREE_CODE_CLASS (code_right) at all. 

it could do that, but it seems to be never necessary. I've tried to construct 
a testcase, but the tree seems to be always built on the left (because 
comparison operators are left associative ?!). I've added the code_right 
variant now to the patch as well, no practical difference as far as I can 
see. 

> I also don't understand why you require that 
> code_left != NE_EXPR && code_left != EQ_EXPR.  I don't even see how
> that case could arise.

it can happen on a == b == c chains that it matches there as well (even though 
there was already a warning emitted before). 

> I think it would be cleaner to use an else:
>   if (code == EQ_EXPR || code == NE_EXPR)
>     ...
>   else if (TREE_CODE_CLASS (code) == tcc_comparison)

Ok, done now. 

> However, I can not approve this patch to the C/C++ frontends.

Many thanks for reviewing anyway and providing feedback! I'm not sure who is 
able to approve it anyway. 

Here is the updated patch. bootstrapped, regression tested with all languages 
with no additional failures on i686-suse-linux. 


2007-01-12  Dirk Mueller  <dmueller@suse.de>

        * c-common.c (warn_about_parentheses): Separate warning about
        un-parenthized sequence of comparison operators from the one
        which is supposed to warn about x <= y <= z.

        * testsuite/gcc.dg/Wparentheses-2.c: Add new tests.

--- gcc/c-common.c	(revision 120684)
+++ gcc/c-common.c	(working copy)
@@ -6752,12 +6751,23 @@ warn_about_parentheses (enum tree_code c
 		 "suggest parentheses around comparison in operand of &");
     }
 
-  /* Similarly, check for cases like 1<=i<=10 that are probably errors.  */
-  if (TREE_CODE_CLASS (code) == tcc_comparison
-      && (TREE_CODE_CLASS (code_left) == tcc_comparison
-	  || TREE_CODE_CLASS (code_right) == tcc_comparison))
-    warning (OPT_Wparentheses, "comparisons like X<=Y<=Z do not "
-	     "have their mathematical meaning");
+  if (code == EQ_EXPR || code == NE_EXPR)
+    {
+      if (TREE_CODE_CLASS (code_left) == tcc_comparison
+          || TREE_CODE_CLASS (code_right) == tcc_comparison)
+	warning (OPT_Wparentheses,
+		 "suggest parentheses around comparison in operand of %s",
+                 code == EQ_EXPR ? "==" : "!=");
+    }
+  else if (TREE_CODE_CLASS (code) == tcc_comparison)
+    {
+      if ((TREE_CODE_CLASS (code_left) == tcc_comparison
+	   && code_left != NE_EXPR && code_left != EQ_EXPR)
+	  || (TREE_CODE_CLASS (code_right) == tcc_comparison
+	      && code_right != NE_EXPR && code_right != EQ_EXPR))
+	warning (OPT_Wparentheses, "comparisons like X<=Y<=Z do not "
+		 "have their mathematical meaning");
+    }
 }
 
 
Index: gcc/testsuite/gcc.dg/Wparentheses-2.c
===================================================================
--- gcc/testsuite/gcc.dg/Wparentheses-2.c	(revision 120684)
+++ gcc/testsuite/gcc.dg/Wparentheses-2.c	(working copy)
@@ -64,4 +64,52 @@ bar (int a, int b, int c)
   foo (1 != 2 != 3); /* { dg-warning "comparison" "correct warning" } */
   foo ((1 != 2) != 3);
   foo (1 != (2 != 3));
+  foo (a < b == c);  /* { dg-warning "comparison" "correct warning" } */
+  foo ((a < b) == c);
+  foo (a < (b == c));
+  foo (a > b == c);  /* { dg-warning "comparison" "correct warning" } */
+  foo ((a > b) == c);
+  foo (a > (b == c));
+  foo (a == b < c);  /* { dg-warning "comparison" "correct warning" } */
+  foo ((a == b) < c);
+  foo (a == (b < c));
+  foo (a == b > c);  /* { dg-warning "comparison" "correct warning" } */
+  foo ((a == b) > c);
+  foo (a == (b > c));
+  foo (1 < 2 == 3);  /* { dg-warning "comparison" "correct warning" } */
+  foo ((1 < 2) == 3);
+  foo (1 < (2 == 3));
+  foo (1 > 2 == 3);  /* { dg-warning "comparison" "correct warning" } */
+  foo ((1 > 2) == 3);
+  foo (1 > (2 == 3));
+  foo (1 == 2 < 3);  /* { dg-warning "comparison" "correct warning" } */
+  foo ((1 == 2) < 3);
+  foo (1 == (2 < 3));
+  foo (1 == 2 > 3);  /* { dg-warning "comparison" "correct warning" } */
+  foo ((1 == 2) > 3);
+  foo (1 == (2 > 3));
+  foo (a < b != c);  /* { dg-warning "comparison" "correct warning" } */
+  foo ((a < b) != c);
+  foo (a < (b != c));
+  foo (a > b != c);  /* { dg-warning "comparison" "correct warning" } */
+  foo ((a > b) != c);
+  foo (a > (b != c));
+  foo (a != b < c);  /* { dg-warning "comparison" "correct warning" } */
+  foo ((a != b) < c);
+  foo (a != (b < c));
+  foo (a != b > c);  /* { dg-warning "comparison" "correct warning" } */
+  foo ((a != b) > c);
+  foo (a != (b > c));
+  foo (1 < 2 != 3);  /* { dg-warning "comparison" "correct warning" } */
+  foo ((1 < 2) != 3);
+  foo (1 < (2 != 3));
+  foo (1 > 2 != 3);  /* { dg-warning "comparison" "correct warning" } */
+  foo ((1 > 2) != 3);
+  foo (1 > (2 != 3));
+  foo (1 != 2 < 3);  /* { dg-warning "comparison" "correct warning" } */
+  foo ((1 != 2) < 3);
+  foo (1 != (2 < 3));
+  foo (1 != 2 > 3);  /* { dg-warning "comparison" "correct warning" } */
+  foo ((1 != 2) > 3);
+  foo (1 != (2 > 3));
 }