Fix excess warnings from -Wtype-limits with location wrappers (PR c++/88680)

PR c++/88680 reports excess warnings from -Wtype-limits after the C++
FE's use of location wrappers was extended in r267272 for cases such as:

  const unsigned n = 8;
  static_assert (n >= 0 && n % 2 == 0, "");

t.C:3:18: warning: comparison of unsigned expression >= 0 is always true
  [-Wtype-limits]
    3 | static_assert (n >= 0 && n % 2 == 0, "");
      |                ~~^~~~

The root cause is that the location wrapper around "n" breaks the
suppression of the warning for the "if OP0 is a constant that is >= 0"
case.

This patch fixes it by calling fold_for_warn on OP0, extracting the
constant.

Successfully bootstrapped & regrtested on x86_64-pc-linux-gnu.

OK for trunk?

gcc/c-family/ChangeLog:
	PR c++/88680
	* c-common.c (shorten_compare): Call fold_for_warn on op0 when
	implementing -Wtype-limits.

gcc/testsuite/ChangeLog:
	PR c++/88680
	* g++.dg/wrappers/pr88680.C: New test.
---
 gcc/c-family/c-common.c                 |  2 +-
 gcc/testsuite/g++.dg/wrappers/pr88680.C | 47 +++++++++++++++++++++++++++++++++
 2 files changed, 48 insertions(+), 1 deletion(-)
 create mode 100644 gcc/testsuite/g++.dg/wrappers/pr88680.C

Ping:
  https://gcc.gnu.org/ml/gcc-patches/2019-02/msg00363.html

On Wed, 2019-02-06 at 21:23 -0500, David Malcolm wrote:
> PR c++/88680 reports excess warnings from -Wtype-limits after the C++
> FE's use of location wrappers was extended in r267272 for cases such
> as:
> 
>   const unsigned n = 8;
>   static_assert (n >= 0 && n % 2 == 0, "");
> 
> t.C:3:18: warning: comparison of unsigned expression >= 0 is always
> true
>   [-Wtype-limits]
>     3 | static_assert (n >= 0 && n % 2 == 0, "");
>       |                ~~^~~~
> 
> The root cause is that the location wrapper around "n" breaks the
> suppression of the warning for the "if OP0 is a constant that is >=
> 0"
> case.
> 
> This patch fixes it by calling fold_for_warn on OP0, extracting the
> constant.
> 
> Successfully bootstrapped & regrtested on x86_64-pc-linux-gnu.
> 
> OK for trunk?
> 
> gcc/c-family/ChangeLog:
> 	PR c++/88680
> 	* c-common.c (shorten_compare): Call fold_for_warn on op0 when
> 	implementing -Wtype-limits.
> 
> gcc/testsuite/ChangeLog:
> 	PR c++/88680
> 	* g++.dg/wrappers/pr88680.C: New test.
> ---
>  gcc/c-family/c-common.c                 |  2 +-
>  gcc/testsuite/g++.dg/wrappers/pr88680.C | 47
> +++++++++++++++++++++++++++++++++
>  2 files changed, 48 insertions(+), 1 deletion(-)
>  create mode 100644 gcc/testsuite/g++.dg/wrappers/pr88680.C
> 
> diff --git a/gcc/c-family/c-common.c b/gcc/c-family/c-common.c
> index d3b5879..5813e0a 100644
> --- a/gcc/c-family/c-common.c
> +++ b/gcc/c-family/c-common.c
> @@ -3114,7 +3114,7 @@ shorten_compare (location_t loc, tree *op0_ptr,
> tree *op1_ptr,
>        /* Here we must do the comparison on the nominal type
>  	 using the args exactly as we received them.  */
>        type = *restype_ptr;
> -      primop0 = op0;
> +      primop0 = fold_for_warn (op0);
>        primop1 = op1;
>  
>        if (!real1 && !real2 && integer_zerop (primop1)
> diff --git a/gcc/testsuite/g++.dg/wrappers/pr88680.C
> b/gcc/testsuite/g++.dg/wrappers/pr88680.C
> new file mode 100644
> index 0000000..86945db
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/wrappers/pr88680.C
> @@ -0,0 +1,47 @@
> +// { dg-do compile { target c++11 } }
> +// { dg-options "-Wtype-limits" }
> +
> +const unsigned N = 8;
> +const unsigned P = 0;
> +
> +enum { FOO, BAR };
> +
> +static_assert (N >= 0 && N % 2 == 0, "");
> +static_assert (FOO >= 0, "");
> +static_assert (FOO >= FOO, "");
> +static_assert (FOO >= P, "");
> +static_assert (BAR >= P, "");
> +static_assert (N >= FOO, "");
> +
> +void test(unsigned n)
> +{
> +  if (N >= 0 && N % 2 == 0, "")
> +    return;
> +  if (FOO >= 0, "")
> +    return;
> +  if (FOO >= FOO, "")
> +    return;
> +  if (FOO >= P, "")
> +    return;
> +  if (BAR >= P)
> +    return;
> +  if (N >= FOO, "")
> +    return;
> +  if (n >= 0) // { dg-warning ">= 0 is always true" }
> +    return;
> +  if (n < 0) // { dg-warning "< 0 is always false" }
> +    return;
> +  if (n >= FOO)
> +    return;
> +  if (n < FOO)
> +    return;
> +  if (N >= 0)
> +    return;
> +  if (N < 0)
> +    return;
> +  if (N >= FOO)
> +    return;
> +  if (N < FOO)
> +    return;
> +
> +}

On 2/6/19 9:23 PM, David Malcolm wrote:
> PR c++/88680 reports excess warnings from -Wtype-limits after the C++
> FE's use of location wrappers was extended in r267272 for cases such as:
> 
>    const unsigned n = 8;
>    static_assert (n >= 0 && n % 2 == 0, "");
> 
> t.C:3:18: warning: comparison of unsigned expression >= 0 is always true
>    [-Wtype-limits]
>      3 | static_assert (n >= 0 && n % 2 == 0, "");
>        |                ~~^~~~
> 
> The root cause is that the location wrapper around "n" breaks the
> suppression of the warning for the "if OP0 is a constant that is >= 0"
> case.
> 
> This patch fixes it by calling fold_for_warn on OP0, extracting the
> constant.

Is there a reason not to do this for OP1 as well?

Jason

On Thu, 2019-02-14 at 10:38 -0500, Jason Merrill wrote:
> On 2/6/19 9:23 PM, David Malcolm wrote:
> > PR c++/88680 reports excess warnings from -Wtype-limits after the
> > C++
> > FE's use of location wrappers was extended in r267272 for cases
> > such as:
> > 
> >    const unsigned n = 8;
> >    static_assert (n >= 0 && n % 2 == 0, "");
> > 
> > t.C:3:18: warning: comparison of unsigned expression >= 0 is always
> > true
> >    [-Wtype-limits]
> >      3 | static_assert (n >= 0 && n % 2 == 0, "");
> >        |                ~~^~~~
> > 
> > The root cause is that the location wrapper around "n" breaks the
> > suppression of the warning for the "if OP0 is a constant that is >=
> > 0"
> > case.
> > 
> > This patch fixes it by calling fold_for_warn on OP0, extracting the
> > constant.
> 
> Is there a reason not to do this for OP1 as well?

There's an asymmetry in the warning; it's looking for a comparison of a
LHS expression against an RHS constant 0, spelled as "0".

If we fold_for_warn on the RHS, then that folding introduces a warning
for expressions that aren't spelled as "0" but can be folded to 0,
e.g., with:

enum { FOO, BAR };

pr88680.C:34:9: warning: comparison of unsigned expression >= 0 is
always true [-Wtype-limits]
   34 |   if (n >= FOO)
      |       ~~^~~~~~
pr88680.C:36:9: warning: comparison of unsigned expression < 0 is
always false [-Wtype-limits]
   36 |   if (n < FOO)
      |       ~~^~~~~

which we didn't warn for before.

We need to fold the LHS so that we can look through wrapper nodes, and
around wrappers around enum values, to suppress the warning if the
folded version of the LHS is a constant that fits in the data type (for
the example given in the original message above).

Dave

On Thu, Feb 14, 2019 at 11:26:15AM -0500, David Malcolm wrote:
> There's an asymmetry in the warning; it's looking for a comparison of a
> LHS expression against an RHS constant 0, spelled as "0".
> 
> If we fold_for_warn on the RHS, then that folding introduces a warning
> for expressions that aren't spelled as "0" but can be folded to 0,
> e.g., with:
> 
> enum { FOO, BAR };

So, shouldn't it be made symmetric?  Check if one argument is literal 0
before folding, and only if it is, fold_for_warn the other argument?

	Jakub