PR c++/36921 [4.3/4.4 Regression] warning "comparison does not have mathematical meaning" is not correct for overloaded operators that do not return boolean

PR c++/36921 [4.3/4.4 Regression] warning "comparison does not have mathematical meaning" is not correct for overloaded operators that do not return boolean

[Manuel López-Ibáñez]

[-- Attachment #1: Type: text/plain, Size: 383 bytes --]

Bootstrapped and regression tested on x86_64-unknown-linux-gnu with
--enable-languages=all,obj-c++ --enable-decimal-float.

OK for trunk and 4.3?

2008-10-25  Manuel López-Ibáñez  <manu@gcc.gnu.org>

	PR c++/36921
cp/
	* typeck.c (build_x_binary_op): Do not warn for overloaded
	comparison operators that do not return boolean.
testsuite/
	* g++.dg/warn/pr36921.C: New.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: fix-pr36921.diff --]
[-- Type: text/x-diff; name=fix-pr36921.diff, Size: 1463 bytes --]

Index: gcc/testsuite/g++.dg/warn/pr36921.C
===================================================================
--- gcc/testsuite/g++.dg/warn/pr36921.C	(revision 0)
+++ gcc/testsuite/g++.dg/warn/pr36921.C	(revision 0)
@@ -0,0 +1,13 @@
+/* PR 36921: comparison operator can be overloaded. Do not emit
+   warnings in such case.
+ { dg-do compile }
+ { dg-options "-Wparentheses" }
+*/
+struct A {};
+A operator<(A, A) { return A(); }
+
+int main() {
+  A() < A() < A(); // should not emit warning
+  1 < 2 < 3; // { dg-warning "mathematical meaning" "parentheses" }
+  return 0;
+}
Index: gcc/cp/typeck.c
===================================================================
--- gcc/cp/typeck.c	(revision 141350)
+++ gcc/cp/typeck.c	(working copy)
@@ -3155,11 +3155,15 @@ build_x_binary_op (enum tree_code code, 
   if (warn_parentheses
       && !processing_template_decl
       && !error_operand_p (arg1)
       && !error_operand_p (arg2)
       && (code != LSHIFT_EXPR
-	  || !CLASS_TYPE_P (TREE_TYPE (arg1))))
+	  || !CLASS_TYPE_P (TREE_TYPE (arg1)))
+      /* Do not warn about overloaded comparison operator that does
+	 not return boolean. */
+      && (TREE_CODE_CLASS (code) != tcc_comparison 
+	  || TREE_CODE (TREE_TYPE (expr)) == BOOLEAN_TYPE))
     warn_about_parentheses (code, arg1_code, orig_arg1, arg2_code, orig_arg2);
 
   if (processing_template_decl && expr != error_mark_node)
     return build_min_non_dep (code, expr, orig_arg1, orig_arg2);
 

Re: PR c++/36921 [4.3/4.4 Regression] warning "comparison does not have mathematical meaning" is not correct for overloaded operators that do not return boolean

[Jakub Jelinek]

On Sat, Oct 25, 2008 at 04:50:59PM +0200, Manuel López-Ibáñez wrote:
> Bootstrapped and regression tested on x86_64-unknown-linux-gnu with
> --enable-languages=all,obj-c++ --enable-decimal-float.
> 
> OK for trunk and 4.3?
> 
> 2008-10-25  Manuel López-Ibáñez  <manu@gcc.gnu.org>
> 
> 	PR c++/36921
> cp/
> 	* typeck.c (build_x_binary_op): Do not warn for overloaded
> 	comparison operators that do not return boolean.
> testsuite/
> 	* g++.dg/warn/pr36921.C: New.

Are you sure we don't want to warn even about the:
  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 ? "==" : "!=");
    }
case if EQ_EXPR or NE_EXPR don't return boolean?
I'd say you want:

> +      /* Do not warn about overloaded comparison operator that does
> +	 not return boolean. */
> +      && (TREE_CODE_CLASS (code) != tcc_comparison 
> +	  || TREE_CODE (TREE_TYPE (expr)) == BOOLEAN_TYPE))

+      /* Do not warn about overloaded comparison operator that does
+	 not return boolean. */
+      && (TREE_CODE_CLASS (code) != tcc_comparison 
+	  || code == EQ_EXPR
+	  || code == NE_EXPR
+	  || TREE_CODE (TREE_TYPE (expr)) == BOOLEAN_TYPE))

>      warn_about_parentheses (code, arg1_code, orig_arg1, arg2_code, orig_arg2);

	Jakub

Re: PR c++/36921 [4.3/4.4 Regression] warning "comparison does not have mathematical meaning" is not correct for overloaded operators that do not return boolean

[Manuel López-Ibáñez]

[-- Attachment #1: Type: text/plain, Size: 880 bytes --]

2008/10/25 Jakub Jelinek <jakub@redhat.com>:
>
> Are you sure we don't want to warn even about the:
>  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 ? "==" : "!=");
>    }
> case if EQ_EXPR or NE_EXPR don't return boolean?

Thanks. I added your suggestion. Bootstrapped and regression tested on
x86_64-unknown-linux-gnu with --enable-languages=all.

OK for trunk?

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

	PR c++/36921
cp/
	* typeck.c (build_x_binary_op): Do not warn for overloaded
	comparison operators that do not return boolean.
testsuite/
	* g++.dg/warn/pr36921.C: New.

[-- Attachment #2: fix-pr36921.diff --]
[-- Type: text/plain, Size: 2200 bytes --]

Index: gcc/testsuite/g++.dg/warn/pr36921.C
===================================================================
--- gcc/testsuite/g++.dg/warn/pr36921.C	(revision 0)
+++ gcc/testsuite/g++.dg/warn/pr36921.C	(revision 0)
@@ -0,0 +1,27 @@
+/* PR 36921: comparison operator can be overloaded. Do not emit
+   warnings in such case.
+ { dg-do compile }
+ { dg-options "-Wparentheses" }
+*/
+struct A {};
+A operator<(A, A) { return A(); }
+A operator>(A, A) { return A(); }
+A operator<=(A, A) { return A(); }
+A operator>=(A, A) { return A(); }
+A operator==(A, A) { return A(); }
+A operator!=(A, A) { return A(); }
+
+int main() {
+  A() < A() < A(); // should not emit warning
+  1 < 2 < 3; // { dg-warning "mathematical meaning" "parentheses" }
+  A() > A() > A(); // should not emit warning
+  1 > 2 > 3; // { dg-warning "mathematical meaning" "parentheses" }
+  A() <= A() <= A(); // should not emit warning
+  1 <= 2 <= 3; // { dg-warning "mathematical meaning" "parentheses" }
+  A() >= A() >= A(); // should not emit warning
+  1 >= 2 >= 3; // { dg-warning "mathematical meaning" "parentheses" }
+
+  A() == A() < A (); // { dg-warning "suggest parentheses" "parentheses" }
+  A() < A() != A (); // { dg-warning "suggest parentheses" "parentheses" }
+  return 0;
+}
Index: gcc/cp/typeck.c
===================================================================
--- gcc/cp/typeck.c	(revision 141531)
+++ gcc/cp/typeck.c	(working copy)
@@ -3155,11 +3155,17 @@ build_x_binary_op (enum tree_code code, 
   if (warn_parentheses
       && !processing_template_decl
       && !error_operand_p (arg1)
       && !error_operand_p (arg2)
       && (code != LSHIFT_EXPR
-	  || !CLASS_TYPE_P (TREE_TYPE (arg1))))
+	  || !CLASS_TYPE_P (TREE_TYPE (arg1)))
+      /* Do not warn about overloaded comparison operator that does
+	 not return boolean.  */
+      && (TREE_CODE_CLASS (code) != tcc_comparison 
+	  || code == EQ_EXPR 
+	  || code == NE_EXPR
+	  || TREE_CODE (TREE_TYPE (expr)) == BOOLEAN_TYPE))
     warn_about_parentheses (code, arg1_code, orig_arg1, arg2_code, orig_arg2);
 
   if (processing_template_decl && expr != error_mark_node)
     return build_min_non_dep (code, expr, orig_arg1, orig_arg2);
 

Re: PR c++/36921 [4.3/4.4 Regression] warning "comparison does not have mathematical meaning" is not correct for overloaded operators that do not return boolean

[Mark Mitchell]

Manuel López-Ibáñez wrote:

> 2008-11-04  Manuel López-Ibáñez  <manu@gcc.gnu.org>
> 
> 	PR c++/36921
> cp/
> 	* typeck.c (build_x_binary_op): Do not warn for overloaded
> 	comparison operators that do not return boolean.
> testsuite/
> 	* g++.dg/warn/pr36921.C: New.

As noted in the PR, I'm not entirely convinced this is a desirable change.

-- 
Mark Mitchell
CodeSourcery
mark@codesourcery.com
(650) 331-3385 x713

Re: PR c++/36921 [4.3/4.4 Regression] warning "comparison does not have mathematical meaning" is not correct for overloaded operators that do not return boolean

[Jason Merrill]

I agree that we don't want to warn for this code, but I think the right 
place to fix it is in warn_about_parentheses; your patch will also 
disable the parenthesis warnings for overloaded comparison ops, which we 
don't want, since precedence is the same regardless of the return type.

Jason

Re: PR c++/36921 [4.3/4.4 Regression] warning "comparison does not have mathematical meaning" is not correct for overloaded operators that do not return boolean

[Jakub Jelinek]

On Fri, Dec 19, 2008 at 05:47:36PM -0500, Jason Merrill wrote:
> I agree that we don't want to warn for this code, but I think the right  
> place to fix it is in warn_about_parentheses; your patch will also  
> disable the parenthesis warnings for overloaded comparison ops, which we  
> don't want, since precedence is the same regardless of the return type.

warn_about_parentheses isn't passed the whole expr though, so it can't check
its type.

Manu's patch just stopped calling warn_about_parentheses for (non-bool
returning overloaded) comparison other than ==/!=, but when code
has tcc_comparison class other than EQ_EXPR/NE_EXPR,
    default:
      if (TREE_CODE_CLASS (code) == tcc_comparison
           && ((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");
is the only thing warn_about_parentheses warns about.
We already have a precedent in build_x_binary_op for avoiding
warn_about_parentheses call in other situations, in particular about
obj << x + y.

	Jakub

Re: PR c++/36921 [4.3/4.4 Regression] warning "comparison does not have mathematical meaning" is not correct for overloaded operators that do not return boolean

[Jason Merrill]

Jakub Jelinek wrote:
> On Fri, Dec 19, 2008 at 05:47:36PM -0500, Jason Merrill wrote:
>> I agree that we don't want to warn for this code, but I think the right  
>> place to fix it is in warn_about_parentheses; your patch will also  
>> disable the parenthesis warnings for overloaded comparison ops, which we  
>> don't want, since precedence is the same regardless of the return type.
> 
> warn_about_parentheses isn't passed the whole expr though, so it can't check
> its type.

True, but that's not important.  The interesting thing is the types of 
the arguments; we only want to warn if one of the arguments is a truthvalue.

> when code has tcc_comparison class other than EQ_EXPR/NE_EXPR,
>         warning (OPT_Wparentheses, "comparisons like %<X<=Y<=Z%> do not "
>                  "have their mathematical meaning");
> is the only thing warn_about_parentheses warns about.

Ah, true.

> We already have a precedent in build_x_binary_op for avoiding
> warn_about_parentheses call in other situations, in particular about
> obj << x + y.

Yes, but that's because the C precedence of << seems natural for 
iostreams operations, whereas it can be confusing for arithmetic.

Since, as you point out, we aren't actually missing any wanted warnings, 
I don't mind putting this logic in warn_about_parentheses.  But the test 
should still check the types of the arguments rather than the whole 
expression.

Mark: I understand your objection on the basis of user-defined Boolean 
types, but I think that the false positive rate for this warning for 
arbitrary overloads such as the PR submitter's is likely to outweigh the 
false negatives for Boolean classes.

Jason

Re: PR c++/36921 [4.3/4.4 Regression] warning "comparison does not have mathematical meaning" is not correct for overloaded operators that do not return boolean

[Mark Mitchell]

Jason Merrill wrote:

> Mark: I understand your objection on the basis of user-defined Boolean
> types, but I think that the false positive rate for this warning for
> arbitrary overloads such as the PR submitter's is likely to outweigh the
> false negatives for Boolean classes.

I'm just not sure.  That also means I don't object if everyone else
feels that it makes sense to avoid the warning. :-)

Thanks,

-- 
Mark Mitchell
CodeSourcery
mark@codesourcery.com
(650) 331-3385 x713

Re: PR c++/36921 [4.3/4.4 Regression] warning "comparison does not have mathematical meaning" is not correct for overloaded operators that do not return boolean

[Jakub Jelinek]

On Fri, Dec 19, 2008 at 06:29:48PM -0500, Jason Merrill wrote:
> Since, as you point out, we aren't actually missing any wanted warnings,  
> I don't mind putting this logic in warn_about_parentheses.  But the test  
> should still check the types of the arguments rather than the whole  
> expression.

You're right, that works too.  I've so far just checked this testcase,
will do full bootstrap/regtest soon.

2008-11-04  Jakub Jelinek  <jakub@redhat.com>
	    Manuel López-Ibáñez  <manu@gcc.gnu.org>

	PR c++/36921
	* c-common.c (warn_about_parentheses): Remove ARG_UNUSED from
	arg_left.  Don't warn about X<=Y<=Z if comparison's type isn't
	boolean.

	* g++.dg/warn/pr36921.C: New.

--- gcc/c-common.c.jj	2008-11-15 11:13:56.000000000 +0100
+++ gcc/c-common.c	2008-12-20 00:40:03.000000000 +0100
@@ -8059,7 +8059,7 @@ warn_array_subscript_with_type_char (tre
 
 void
 warn_about_parentheses (enum tree_code code,
-			enum tree_code code_left, tree ARG_UNUSED (arg_left),
+			enum tree_code code_left, tree arg_left,
 			enum tree_code code_right, tree arg_right)
 {
   if (!warn_parentheses)
@@ -8169,9 +8169,11 @@ warn_about_parentheses (enum tree_code c
     default:
       if (TREE_CODE_CLASS (code) == tcc_comparison
 	   && ((TREE_CODE_CLASS (code_left) == tcc_comparison
-		&& code_left != NE_EXPR && code_left != EQ_EXPR)
+		&& code_left != NE_EXPR && code_left != EQ_EXPR
+		&& TREE_CODE (TREE_TYPE (arg_left)) == BOOLEAN_TYPE)
 	       || (TREE_CODE_CLASS (code_right) == tcc_comparison
-		   && code_right != NE_EXPR && code_right != EQ_EXPR)))
+		   && code_right != NE_EXPR && code_right != EQ_EXPR
+		   && TREE_CODE (TREE_TYPE (arg_right)) == BOOLEAN_TYPE)))
 	warning (OPT_Wparentheses, "comparisons like %<X<=Y<=Z%> do not "
 		 "have their mathematical meaning");
       return;
--- gcc/testsuite/g++.dg/warn/pr36921.C.jj	2008-12-20 00:37:34.000000000 +0100
+++ gcc/testsuite/g++.dg/warn/pr36921.C	2008-12-20 00:37:34.000000000 +0100
@@ -0,0 +1,27 @@
+/* PR 36921: comparison operator can be overloaded. Do not emit
+   warnings in such case.
+ { dg-do compile }
+ { dg-options "-Wparentheses" }
+*/
+struct A {};
+A operator<(A, A) { return A(); }
+A operator>(A, A) { return A(); }
+A operator<=(A, A) { return A(); }
+A operator>=(A, A) { return A(); }
+A operator==(A, A) { return A(); }
+A operator!=(A, A) { return A(); }
+
+int main() {
+  A() < A() < A(); // should not emit warning
+  1 < 2 < 3; // { dg-warning "mathematical meaning" "parentheses" }
+  A() > A() > A(); // should not emit warning
+  1 > 2 > 3; // { dg-warning "mathematical meaning" "parentheses" }
+  A() <= A() <= A(); // should not emit warning
+  1 <= 2 <= 3; // { dg-warning "mathematical meaning" "parentheses" }
+  A() >= A() >= A(); // should not emit warning
+  1 >= 2 >= 3; // { dg-warning "mathematical meaning" "parentheses" }
+
+  A() == A() < A (); // { dg-warning "suggest parentheses" "parentheses" }
+  A() < A() != A (); // { dg-warning "suggest parentheses" "parentheses" }
+  return 0;
+}


	Jakub

Re: PR c++/36921 [4.3/4.4 Regression] warning "comparison does not have mathematical meaning" is not correct for overloaded operators that do not return boolean

[Jakub Jelinek]

On Sat, Dec 20, 2008 at 12:47:20AM +0100, Jakub Jelinek wrote:
> On Fri, Dec 19, 2008 at 06:29:48PM -0500, Jason Merrill wrote:
> > Since, as you point out, we aren't actually missing any wanted warnings,  
> > I don't mind putting this logic in warn_about_parentheses.  But the test  
> > should still check the types of the arguments rather than the whole  
> > expression.
> 
> You're right, that works too.  I've so far just checked this testcase,
> will do full bootstrap/regtest soon.

While it works for C++, it doesn't for C, where the type of the comparison
argument is INTEGER_TYPE.  I guess using INTEGRAL_TYPE_P instead should work
though.

	Jakub

Re: PR c++/36921 [4.3/4.4 Regression] warning "comparison does not have mathematical meaning" is not correct for overloaded operators that do not return boolean

[Jakub Jelinek]

On Sat, Dec 20, 2008 at 12:47:20AM +0100, Jakub Jelinek wrote:
> You're right, that works too.  I've so far just checked this testcase,
> will do full bootstrap/regtest soon.

This passed bootstrap/regtest on x86_64-linux and works for both C and C++.

2008-12-20  Jakub Jelinek  <jakub@redhat.com>
	    Manuel López-Ibáñez  <manu@gcc.gnu.org>

	PR c++/36921
	* c-common.c (warn_about_parentheses): Remove ARG_UNUSED from
	arg_left.  Don't warn about X<=Y<=Z if comparison's type isn't
	integral.

	* g++.dg/warn/pr36921.C: New.

--- gcc/c-common.c.jj	2008-11-15 11:13:56.000000000 +0100
+++ gcc/c-common.c	2008-12-20 01:34:20.000000000 +0100
@@ -8059,7 +8059,7 @@ warn_array_subscript_with_type_char (tre
 
 void
 warn_about_parentheses (enum tree_code code,
-			enum tree_code code_left, tree ARG_UNUSED (arg_left),
+			enum tree_code code_left, tree arg_left,
 			enum tree_code code_right, tree arg_right)
 {
   if (!warn_parentheses)
@@ -8169,9 +8169,11 @@ warn_about_parentheses (enum tree_code c
     default:
       if (TREE_CODE_CLASS (code) == tcc_comparison
 	   && ((TREE_CODE_CLASS (code_left) == tcc_comparison
-		&& code_left != NE_EXPR && code_left != EQ_EXPR)
+		&& code_left != NE_EXPR && code_left != EQ_EXPR
+		&& INTEGRAL_TYPE_P (TREE_TYPE (arg_left)))
 	       || (TREE_CODE_CLASS (code_right) == tcc_comparison
-		   && code_right != NE_EXPR && code_right != EQ_EXPR)))
+		   && code_right != NE_EXPR && code_right != EQ_EXPR
+		   && INTEGRAL_TYPE_P (TREE_TYPE (arg_right)))))
 	warning (OPT_Wparentheses, "comparisons like %<X<=Y<=Z%> do not "
 		 "have their mathematical meaning");
       return;
--- gcc/testsuite/g++.dg/warn/pr36921.C.jj	2008-12-20 00:37:34.000000000 +0100
+++ gcc/testsuite/g++.dg/warn/pr36921.C	2008-12-20 00:37:34.000000000 +0100
@@ -0,0 +1,27 @@
+/* PR 36921: comparison operator can be overloaded. Do not emit
+   warnings in such case.
+ { dg-do compile }
+ { dg-options "-Wparentheses" }
+*/
+struct A {};
+A operator<(A, A) { return A(); }
+A operator>(A, A) { return A(); }
+A operator<=(A, A) { return A(); }
+A operator>=(A, A) { return A(); }
+A operator==(A, A) { return A(); }
+A operator!=(A, A) { return A(); }
+
+int main() {
+  A() < A() < A(); // should not emit warning
+  1 < 2 < 3; // { dg-warning "mathematical meaning" "parentheses" }
+  A() > A() > A(); // should not emit warning
+  1 > 2 > 3; // { dg-warning "mathematical meaning" "parentheses" }
+  A() <= A() <= A(); // should not emit warning
+  1 <= 2 <= 3; // { dg-warning "mathematical meaning" "parentheses" }
+  A() >= A() >= A(); // should not emit warning
+  1 >= 2 >= 3; // { dg-warning "mathematical meaning" "parentheses" }
+
+  A() == A() < A (); // { dg-warning "suggest parentheses" "parentheses" }
+  A() < A() != A (); // { dg-warning "suggest parentheses" "parentheses" }
+  return 0;
+}

	Jakub

Re: PR c++/36921 [4.3/4.4 Regression] warning "comparison does not have mathematical meaning" is not correct for overloaded operators that do not return boolean

[Jason Merrill]

OK.

Jason