On 12/16/2017 03:01 PM, Martin Sebor wrote:
On 12/16/2017 06:12 AM, David Malcolm wrote:
On Mon, 2017-12-11 at 18:36 -0500, Jason Merrill wrote:
On 11/10/2017 04:45 PM, David Malcolm wrote:
We need to strip away location wrappers in tree.c predicates like
integer_zerop, otherwise they fail when they're called on
wrapped INTEGER_CST; an example can be seen for
c-c++-common/Wmemset-transposed-args1.c
in g++.sum, where the warn_for_memset fails to detect integer zero
if the location wrappers aren't stripped.
These shouldn't be needed; callers should have folded away location
wrappers. I would hope for STRIP_ANY_LOCATION_WRAPPER to be almost
never needed.
warn_for_memset may be missing some calls to fold_for_warn.
It is, thanks.
Here's a revised fix for e.g. Wmemset-transposed-args1.c, replacing
"[PATCH 05/14] tree.c: strip location wrappers from integer_zerop etc"
and
"[PATCH 10/14] warn_for_memset: handle location wrappers"
This version of the patch simply calls fold_for_warn on each of the
arguments before calling warn_for_memset. This ensures that
warn_for_memset detects integer zero. It also adds a
literal_integer_zerop to deal with location wrapper nodes when
building "literal_mask" for the call, since this must be called
*before* the fold_for_warn calls.
Successfully bootstrapped®rtested on x86_64-pc-linux-gnu, as
part of the kit.
Is this OK for trunk, once the rest of the kit is approved?
gcc/cp/ChangeLog:
* parser.c (literal_integer_zerop): New function.
(cp_parser_postfix_expression): When calling warn_for_memset,
replace integer_zerop calls with literal_integer_zerop, and
call fold_for_warn on the arguments.
---
gcc/cp/parser.c | 18 ++++++++++++++++--
1 file changed, 16 insertions(+), 2 deletions(-)
diff --git a/gcc/cp/parser.c b/gcc/cp/parser.c
index d15769a..7bca460 100644
--- a/gcc/cp/parser.c
+++ b/gcc/cp/parser.c
@@ -6621,6 +6621,16 @@ cp_parser_compound_literal_p (cp_parser *parser)
return compound_literal_p;
}
+/* Return 1 if EXPR is the integer constant zero or a complex constant
+ of zero, without any folding, but ignoring location wrappers. */
+
+static int
+literal_integer_zerop (const_tree expr)
+{
+ STRIP_ANY_LOCATION_WRAPPER (expr);
+ return integer_zerop (expr);
+}
+
/* Parse a postfix-expression.
postfix-expression:
@@ -7168,8 +7178,12 @@ cp_parser_postfix_expression (cp_parser
*parser, bool address_p, bool cast_p,
tree arg0 = (*args)[0];
tree arg1 = (*args)[1];
tree arg2 = (*args)[2];
- int literal_mask = ((!!integer_zerop (arg1) << 1)
- | (!!integer_zerop (arg2) << 2));
+ /* Handle any location wrapper nodes. */
+ arg0 = fold_for_warn (arg0);
+ int literal_mask = ((!!literal_integer_zerop (arg1) << 1)
+ | (!!literal_integer_zerop (arg2) << 2));
The double negation jumped out at me. The integer_zerop() function
looks like a predicate that hasn't yet been converted to return bool.
It seems that new predicates that are implemented on top of it could
return bool and their callers avoid this conversion. (At some point
in the future it would also be nice to convert the existing
predicates to return bool).
Agreed. And since integer_zerop in fact returns boolean values, there
seems to be no need for the double negative in the first place.
So, please make the new function return bool and remove the !!s.
And I think the fold_for_warn call should be in warn_for_memset, and it
should be called for arg2 as well instead of specifically handling
CONST_DECL Here.
Jason
Jason
