On Fri, Jul 11, 2025 at 12:26:54PM -0400, Jason Merrill wrote:
> On 7/11/25 9:09 AM, Jakub Jelinek wrote:
> > On Thu, Jul 10, 2025 at 04:35:49PM -0400, Jason Merrill wrote:
> > > > --- gcc/cp/cp-gimplify.cc.jj 2025-04-12 21:41:42.660924514 +0200
> > > > +++ gcc/cp/cp-gimplify.cc 2025-04-23 21:33:19.050931604 +0200
> > > > @@ -3200,7 +3200,23 @@ cp_fold (tree x, fold_flags_t flags)
> > > > loc = EXPR_LOCATION (x);
> > > > op0 = cp_fold_maybe_rvalue (TREE_OPERAND (x, 0), rval_ops,
> > > > flags);
> > > > + bool clear_decl_read;
> > > > + clear_decl_read = false;
> > > > + if (code == MODIFY_EXPR
> > > > + && (VAR_P (op0) || TREE_CODE (op0) == PARM_DECL)
> > > > + && !DECL_READ_P (op0)
> > > > + && (VAR_P (op0) ? warn_unused_but_set_variable
> > > > + : warn_unused_but_set_parameter) > 2
> > > > + && BINARY_CLASS_P (TREE_OPERAND (x, 1))
> > > > + && TREE_OPERAND (TREE_OPERAND (x, 1), 0) == op0)
> > > > + {
> > > > + mark_exp_read (TREE_OPERAND (TREE_OPERAND (x, 1), 1));
> > > > + if (!DECL_READ_P (op0))
> > > > + clear_decl_read = true;
> > > > + }
> > > > op1 = cp_fold_rvalue (TREE_OPERAND (x, 1), flags);
> > > > + if (clear_decl_read)
> > > > + DECL_READ_P (op0) = 0;
> > >
> > > Why does this need to happen in cp_fold? Weren't the flags set properly
> > > at
> > > build time?
> >
> > Without the cp-gimplify.cc (cp_fold) hunk there are tons of FAILs, with
>
> Then could we do something simpler at this point, just preserving the
> pre-folding state of DECL_READ_P (op0) without looking at the RHS?
That would be wrong.
The goal of the patch is to just add some extra exceptions which aren't
counted as uses.
If a MODIFY_EXPR is say
op0 = foo (op0);
then we shouldn't clear DECL_READ_P, the call does or could read the value
and use it for something, so warning that op0 is just set but not used would
be false positive.
The intent of the extension is to handle
op0 @= expr;
where expr doesn't refer to op0, or some cases of
op0 = op0 @ expr;
Still, if it is e.g.
op0 @= foo (op0);
we want to have DECL_READ_P set.
Which is why the above code when it sees op0 = op0 @ expr
calls mark_exp_read on expr and if it doesn't set DECL_READ_P (op0),
will clear it after cp_fold_rvalue on the whole rhs.
> > > > @@ -3740,7 +3740,28 @@ finish_unary_op_expr (location_t op_loc,
> > > > tree expr_ovl = expr;
> > > > if (!processing_template_decl)
> > > > - expr_ovl = cp_fully_fold (expr_ovl);
> > > > + switch (code)
> > > > + {
> > > > + case PREINCREMENT_EXPR:
> > > > + case PREDECREMENT_EXPR:
> > > > + case POSTINCREMENT_EXPR:
> > > > + case POSTDECREMENT_EXPR:
> > > > + tree stripped_expr;
> > > > + stripped_expr = tree_strip_any_location_wrapper (expr);
> > > > + if ((VAR_P (stripped_expr) || TREE_CODE (stripped_expr) ==
> > > > PARM_DECL)
> > > > + && !DECL_READ_P (stripped_expr)
> > > > + && (VAR_P (stripped_expr) ? warn_unused_but_set_variable
> > > > + : warn_unused_but_set_parameter)
> > > > > 1)
> > > > + {
> > > > + expr_ovl = cp_fully_fold (expr_ovl);
> > >
> > > Again I wonder why cp_fold is setting DECL_READ_P.
> >
> > See above.
>
> Ah, the problem here is that cp_fully_fold assumes that we're using its
> operand as an rvalue, which is wrong for ++/--. And we aren't going to get
> a constant result anyway, so finish_unary_op_expr shouldn't call
> cp_fully_fold, it should return result early for lvalue codes.
Well, I'm not sure it is actually an error. finish_unary_op_expr doesn't
use cp_fully_fold result as an operand of {PRE,POST}{INC,DEC}REMENT_EXPR
(that would be wrong, we don't want the operand to fold into non-lvalue), it
is called only to find out if overflow warning should be emitted. And I
think these ops act as both lvalue and rvalue, they read the old value,
increment/decrement it and store the new value. The patch just ensures that
we don't consider it a DECL_READ_P in some cases.
The way the patch was written was for each of the exception cases find out
what sets DECL_READ_P even when I'd like it not to be set, and tweak those
spots, so that it is not set.
Jakub
