On Thu, 2019-02-14 at 17:32 +0100, Jakub Jelinek wrote:
> 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
The reference to symmetry in my earlier email was somewhat
misleading, sorry.
The test happens after a canonicalization of the ordering happens
here, near the top of shorten_compare:
/* If first arg is constant, swap the args (changing operation
so value is preserved), for canonicalization. Don't do this if
the second arg is 0. */
so this already gives us symmetry.
Here's an updated version of the patch which add the fold_for_warn in
a slightly later place, and adds a comment, and some more test cases.
Successfully bootstrapped & regrtested on x86_64-pc-linux-gnu.
OK for trunk?
Blurb from v1:
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.
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 | 13 ++++++--
gcc/testsuite/g++.dg/wrappers/pr88680.C | 56 +++++++++++++++++++++++++++++++++
2 files changed, 66 insertions(+), 3 deletions(-)
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 ae23e59..c6856c9 100644
--- a/gcc/c-family/c-common.c
+++ b/gcc/c-family/c-common.c
@@ -3117,6 +3117,12 @@ shorten_compare (location_t loc, tree *op0_ptr, tree
*op1_ptr,
primop0 = op0;
primop1 = op1;
+ /* We want to fold unsigned comparisons of >= and < against zero.
+ For these, we may also issue a warning if we have a non-constant
+ compared against zero, where the zero was spelled as "0" (rather
+ than merely folding to it).
+ If we have at least one constant, then op1 is constant
+ and we may have a non-constant expression as op0. */
if (!real1 && !real2 && integer_zerop (primop1)
&& TYPE_UNSIGNED (*restype_ptr))
{
@@ -3125,13 +3131,14 @@ shorten_compare (location_t loc, tree *op0_ptr, tree
*op1_ptr,
if OP0 is a constant that is >= 0, the signedness of
the comparison isn't an issue, so suppress the
warning. */
+ tree folded_op0 = fold_for_warn (op0);
bool warn =
warn_type_limits && !in_system_header_at (loc)
- && !(TREE_CODE (primop0) == INTEGER_CST
+ && !(TREE_CODE (folded_op0) == INTEGER_CST
&& !TREE_OVERFLOW (convert (c_common_signed_type (type),
- primop0)))
+ folded_op0)))
/* Do not warn for enumeration types. */
- && (TREE_CODE (expr_original_type (primop0)) != ENUMERAL_TYPE);
+ && (TREE_CODE (expr_original_type (folded_op0)) != ENUMERAL_TYPE);
switch (code)
{
diff --git a/gcc/testsuite/g++.dg/wrappers/pr88680.C
b/gcc/testsuite/g++.dg/wrappers/pr88680.C
new file mode 100644
index 0000000..5497cda
--- /dev/null
+++ b/gcc/testsuite/g++.dg/wrappers/pr88680.C
@@ -0,0 +1,56 @@
+// { 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;
+ if (0 <= FOO)
+ return;
+ if (0 <= n) // { dg-warning ">= 0 is always true" }
+ return;
+ if (0 > n) // { dg-warning "< 0 is always false" }
+ return;
+ if (N <= FOO)
+ return;
+ if (N <= n)
+ return;
+}
--
1.8.5.3
