On Fri, Sep 15, 2023 at 02:08:46PM -0400, Jason Merrill wrote:
> On 9/13/23 20:02, Marek Polacek wrote:
> > On Wed, Sep 13, 2023 at 05:57:47PM -0400, Jason Merrill wrote:
> > > On 9/13/23 16:56, Marek Polacek wrote:
> > > > On Tue, Sep 12, 2023 at 05:26:25PM -0400, Jason Merrill wrote:
> > > > > On 9/8/23 14:24, Marek Polacek wrote:
> > > > > > + switch (TREE_CODE (stmt))
> > > > > > +{
> > > > > > +/* Unfortunately we must handle code like
> > > > > > +false ? bar () : 42
> > > > > > + where we have to check bar too. */
> > > > > > +case COND_EXPR:
> > > > > > + if (cp_fold_immediate_r (&TREE_OPERAND (stmt, 1),
> > > > > > walk_subtrees, data))
> > > > > > + return error_mark_node;
> > > > > > + if (TREE_OPERAND (stmt, 2)
> > > > > > + && cp_fold_immediate_r (&TREE_OPERAND (stmt, 2),
> > > > > > walk_subtrees, data))
> > > > > > + return error_mark_node;
> > > > >
> > > > > Is this necessary? Doesn't walk_tree already walk into the arms of
> > > > > COND_EXPR?
> > > >
> > > > Unfortunately yes. The cp_fold call in cp_fold_r could fold the ?: into
> > > > a constant before we see it here. I've added a comment saying just
> > > > that.
> > >
> > > Ah. But in that case I guess we need to walk into the arms, not just
> > > check
> > > the top-level expression in them.
> > Arg, of course. I was fooled into thinking that it would recurse, but
> > you're right. Fixed by using cp_walk_tree as I intended. Tested in
> > consteval34.C.
> >
> > > But maybe cp_fold_r should do that before the cp_fold, instead of this
> > > function?
> >
> > I...am not sure how that would be better than what I did.
>
> Callers of cp_fold_immediate don't need this because cp_fold_r isn't
> involved, so it isn't folding anything.
This is true.
> cp_fold_r can walk the arms with cp_fold_r and then clear *walk_subtrees to
> avoid walking the arms again normally.
I didn't think we wanted to do everything cp_fold_r does even in dead
branches, but ok.
I figured that we need to recurse on the conditional of the COND_EXPR too.
It can be, for example:
&((const struct S *) VIEW_CONVERT_EXPR(q))->b != 0B
which is supposed to be evaluated into:
&((const struct S *) &s)->b != 0B
otherwise we'd get ICEs like "error: constant not recomputed when 'ADDR_EXPR'
changed".
> cp_fold_r uses data->pset to avoid walking the same tree twice;
> cp_fold_immediate_r currently doesn't do anything to avoid that. If
> cp_fold_immediate_r doesn't itself call cp_walk_tree, cp_fold_immediate can
> use cp_walk_tree_without_duplicates.
Adjusted.
> > > > > > + break;
> > > > > > +
> > > > > > case PTRMEM_CST:
> > > > > > if (TREE_CODE (PTRMEM_CST_MEMBER (stmt)) == FUNCTION_DECL
> > > > > > && DECL_IMMEDIATE_FUNCTION_P (PTRMEM_CST_MEMBER
> > > > > > (stmt)))
> > > > > > {
> > > > > > - if (!data->pset.add (stmt))
> > > > > > + if (!data->pset.add (stmt) && (complain & tf_error))
> > > > > > error_at (PTRMEM_CST_LOCATION (stmt),
> > > > > > "taking address of an immediate function
> > > > > > %qD",
> > > > > > PTRMEM_CST_MEMBER (stmt));
> > > > > > stmt = *stmt_p = build_zero_cst (TREE_TYPE (stmt));
> > > > >
> > > > > It looks like this will overwrite *stmt_p even if we didn't give an
> > > > > error.
> > > >
> > > > I suppose that could result in missing errors, adjusted. And there's no
> > > > point in setting stmt.
> > > > > > - break;
> > > > > > + return error_mark_node;
> > > > > > }
> > > > > > break;
> > > > > > +/* Expand immediate invocations. */
> > > > > > +case CALL_EXPR:
> > > > > > +case AGGR_INIT_EXPR:
> > > > > > + if (tree fn = cp_get_callee (stmt))
> > > > > > + if (TREE_CODE (fn) != ADDR_EXPR || ADDR_EXPR_DENOTES_CALL_P
> > > > > > (fn))
> > > > > > + if (tree fndecl = cp_get_fndecl_from_callee (fn,
> > > > > > /*fold*/false))
> > > > > > + if (DECL_IMMEDIATE_FUNCTION_P (fndecl))
> > > > > > + {
> > > > > > + *stmt_p = stmt = cxx_constant_value (stmt, complain);
> > > > >
> > > > > Likewise.
> > > >
> > > > I think we have to keep setting *stmt_p to actually evaluate consteval
> > > > functions.
> > >
> > > But only when it succeeds; we don't want to set it to error_mark_node if
> > > we
> > > aren't complaining.
> >
> > Hmm, probably not. Fixed, thanks.
> >
> > + if (DECL_IMMEDIATE_FUNCTION_P (fndecl))
> > + {
> > + stmt = cxx_constant_value (stmt, complain);
> > + if (stmt == error_mark_node && (complain & tf_error))
> > + return error_mark_node;
> > + *stmt_p = stmt;
> > + }
>
> This seems backwards; like with the ADDR_EXPR/PTRMEM_CST cases, I think we
> want to return error_mark_node regardless of complain, but only set *stmt_p
> when complaining.
Ug. I really hope it's alright thi
