diff mbox

-Wtautological-compare should be quiet on floats

Message ID 20150729140822.GI3335@redhat.com
State New
Headers show

Commit Message

Marek Polacek July 29, 2015, 2:08 p.m. UTC 
As discussed elsewhere, -Wtautological-compare shouldn't warn about
floating-point types because of the way NaN behave.

I've been meaning to commit this one as obvious, but I'm not sure
whether I should also use HONOR_NANS or whether I can safely ignore
that here.

Bootstrapped/regtested on x86_64-linux, ok for trunk?

2015-07-29  Marek Polacek  <polacek@redhat.com>

	* c-common.c (warn_tautological_cmp): Bail for float types.

	* c-c++-common/Wtautological-compare-3.c: New test.


	Marek

Comments

Marek Polacek July 29, 2015, 2:15 p.m. UTC | #1 
On Wed, Jul 29, 2015 at 04:08:22PM +0200, Marek Polacek wrote:
> As discussed elsewhere, -Wtautological-compare shouldn't warn about
> floating-point types because of the way NaN behave.
> 
> I've been meaning to commit this one as obvious, but I'm not sure
> whether I should also use HONOR_NANS or whether I can safely ignore
> that here.
> 
> Bootstrapped/regtested on x86_64-linux, ok for trunk?
> 
> 2015-07-29  Marek Polacek  <polacek@redhat.com>
> 
> 	* c-common.c (warn_tautological_cmp): Bail for float types.
> 
> 	* c-c++-common/Wtautological-compare-3.c: New test.
> 
> diff --git gcc/c-family/c-common.c gcc/c-family/c-common.c
> index caa801e..9456729 100644
> --- gcc/c-family/c-common.c
> +++ gcc/c-family/c-common.c
> @@ -1910,6 +1910,12 @@ warn_tautological_cmp (location_t loc, enum tree_code code, tree lhs, tree rhs)
>        || (CONVERT_EXPR_P (rhs) || TREE_CODE (rhs) == NON_LVALUE_EXPR))
>      return;
>  
> +  /* Don't warn if either LHS or RHS has an IEEE floating point-type.

Eh, make this "floating-point type".

> +     It could be a NaN, and NaN never compares equal to anything, even
> +     itself.  */
> +  if (FLOAT_TYPE_P (TREE_TYPE (lhs)) || FLOAT_TYPE_P (TREE_TYPE (rhs)))
> +    return;
> +
>    if (operand_equal_p (lhs, rhs, 0))
>      {
>        /* Don't warn about array references with constant indices;
> diff --git gcc/testsuite/c-c++-common/Wtautological-compare-3.c gcc/testsuite/c-c++-common/Wtautological-compare-3.c
> index e69de29..64807b0 100644
> --- gcc/testsuite/c-c++-common/Wtautological-compare-3.c
> +++ gcc/testsuite/c-c++-common/Wtautological-compare-3.c
> @@ -0,0 +1,20 @@
> +/* { dg-do compile } */
> +/* { dg-options "-Wtautological-compare" } */
> +/* Test we don't warn for floats.  */
> +
> +struct S { double d; float f; };
> +
> +void
> +fn1 (int i, float f, double d, struct S *s, float *fp)
> +{
> +  if (f == f);
> +  if (f != f);
> +  if (d == d);
> +  if (d != d);
> +  if (fp[i] == fp[i]);
> +  if (fp[i] != fp[i]);
> +  if (s->f == s->f);
> +  if (s->f != s->f);
> +  if (s->d == s->d);
> +  if (s->d != s->d);
> +}
> 
> 	Marek

	Marek
Jeff Law July 29, 2015, 10:44 p.m. UTC | #2 
On 07/29/2015 08:08 AM, Marek Polacek wrote:
> As discussed elsewhere, -Wtautological-compare shouldn't warn about
> floating-point types because of the way NaN behave.
>
> I've been meaning to commit this one as obvious, but I'm not sure
> whether I should also use HONOR_NANS or whether I can safely ignore
> that here.
>
> Bootstrapped/regtested on x86_64-linux, ok for trunk?
>
> 2015-07-29  Marek Polacek  <polacek@redhat.com>
>
> 	* c-common.c (warn_tautological_cmp): Bail for float types.
>
> 	* c-c++-common/Wtautological-compare-3.c: New test.
I think it comes down to what we think users are going to expect when 
compiling code with NaNs disabled.

One camp would probably say "in my code X == X is always true since I 
don't have NaNs."  The other might say "whether or not to warn on X == X 
should not be dependent on flags such as -ffinite-math-only".

I  could easily make a case for either.  I'd personally tend to lean 
towards the latter.

Jeff
Marek Polacek July 30, 2015, 8:29 a.m. UTC | #3 
On Wed, Jul 29, 2015 at 04:44:46PM -0600, Jeff Law wrote:
> On 07/29/2015 08:08 AM, Marek Polacek wrote:
> >As discussed elsewhere, -Wtautological-compare shouldn't warn about
> >floating-point types because of the way NaN behave.
> >
> >I've been meaning to commit this one as obvious, but I'm not sure
> >whether I should also use HONOR_NANS or whether I can safely ignore
> >that here.
> >
> >Bootstrapped/regtested on x86_64-linux, ok for trunk?
> >
> >2015-07-29  Marek Polacek  <polacek@redhat.com>
> >
> >	* c-common.c (warn_tautological_cmp): Bail for float types.
> >
> >	* c-c++-common/Wtautological-compare-3.c: New test.
> I think it comes down to what we think users are going to expect when
> compiling code with NaNs disabled.
> 
> One camp would probably say "in my code X == X is always true since I don't
> have NaNs."  The other might say "whether or not to warn on X == X should
> not be dependent on flags such as -ffinite-math-only".
> 
> I  could easily make a case for either.  I'd personally tend to lean towards
> the latter.

Me too.  I hope I won't upset anyone by committing this patch now.  I just
don't want glibc folks to trip over this for too long.

Thanks,

	Marek
Richard Biener July 30, 2015, 10:24 a.m. UTC | #4 
On Thu, Jul 30, 2015 at 10:29 AM, Marek Polacek <polacek@redhat.com> wrote:
> On Wed, Jul 29, 2015 at 04:44:46PM -0600, Jeff Law wrote:
>> On 07/29/2015 08:08 AM, Marek Polacek wrote:
>> >As discussed elsewhere, -Wtautological-compare shouldn't warn about
>> >floating-point types because of the way NaN behave.
>> >
>> >I've been meaning to commit this one as obvious, but I'm not sure
>> >whether I should also use HONOR_NANS or whether I can safely ignore
>> >that here.
>> >
>> >Bootstrapped/regtested on x86_64-linux, ok for trunk?
>> >
>> >2015-07-29  Marek Polacek  <polacek@redhat.com>
>> >
>> >     * c-common.c (warn_tautological_cmp): Bail for float types.
>> >
>> >     * c-c++-common/Wtautological-compare-3.c: New test.
>> I think it comes down to what we think users are going to expect when
>> compiling code with NaNs disabled.
>>
>> One camp would probably say "in my code X == X is always true since I don't
>> have NaNs."  The other might say "whether or not to warn on X == X should
>> not be dependent on flags such as -ffinite-math-only".
>>
>> I  could easily make a case for either.  I'd personally tend to lean towards
>> the latter.
>
> Me too.  I hope I won't upset anyone by committing this patch now.  I just
> don't want glibc folks to trip over this for too long.

I agree with doing this unconditionally.

Richard.

> Thanks,
>
>         Marek
diff mbox

Patch

diff --git gcc/c-family/c-common.c gcc/c-family/c-common.c
index caa801e..9456729 100644
--- gcc/c-family/c-common.c
+++ gcc/c-family/c-common.c
@@ -1910,6 +1910,12 @@  warn_tautological_cmp (location_t loc, enum tree_code code, tree lhs, tree rhs)
       || (CONVERT_EXPR_P (rhs) || TREE_CODE (rhs) == NON_LVALUE_EXPR))
     return;
 
+  /* Don't warn if either LHS or RHS has an IEEE floating point-type.
+     It could be a NaN, and NaN never compares equal to anything, even
+     itself.  */
+  if (FLOAT_TYPE_P (TREE_TYPE (lhs)) || FLOAT_TYPE_P (TREE_TYPE (rhs)))
+    return;
+
   if (operand_equal_p (lhs, rhs, 0))
     {
       /* Don't warn about array references with constant indices;
diff --git gcc/testsuite/c-c++-common/Wtautological-compare-3.c gcc/testsuite/c-c++-common/Wtautological-compare-3.c
index e69de29..64807b0 100644
--- gcc/testsuite/c-c++-common/Wtautological-compare-3.c
+++ gcc/testsuite/c-c++-common/Wtautological-compare-3.c
@@ -0,0 +1,20 @@ 
+/* { dg-do compile } */
+/* { dg-options "-Wtautological-compare" } */
+/* Test we don't warn for floats.  */
+
+struct S { double d; float f; };
+
+void
+fn1 (int i, float f, double d, struct S *s, float *fp)
+{
+  if (f == f);
+  if (f != f);
+  if (d == d);
+  if (d != d);
+  if (fp[i] == fp[i]);
+  if (fp[i] != fp[i]);
+  if (s->f == s->f);
+  if (s->f != s->f);
+  if (s->d == s->d);
+  if (s->d != s->d);
+}