On Thu, May 02, 2024 at 01:53:44PM -0400, Jason Merrill wrote:
> On 5/2/24 10:40, Patrick Palka wrote:
> > On Thu, 2 May 2024, Nathaniel Shead wrote:
> >
> > > Bootstrapped and regtested on x86_64-pc-linux-gnu, OK for trunk/14.2?
> > >
> > > Another alternative would be to stream such !DECL_NAME temporaries with
> > > a merge key of MK_unique rather than attempting to find the matching
> > > (nonexistant) field of the class context.
> >
> > Both approaches sound good to me, hard to say which one is preferable..
> >
> > The handling of function-scope vs class-scope temporaries seems to start
> > diverging in:
> >
> > @@ -8861,28 +8861,6 @@ trees_out::decl_node (tree decl, walk_kind ref)
> > return false;
> > }
> > ! tree ctx = CP_DECL_CONTEXT (decl);
> > ! depset *dep = NULL;
> > ! if (streaming_p ())
> > ! dep = dep_hash->find_dependency (decl);
> > ! else if (TREE_CODE (ctx) != FUNCTION_DECL
> > ! || TREE_CODE (decl) == TEMPLATE_DECL
> > ! || DECL_IMPLICIT_TYPEDEF_P (decl)
> > ! || (DECL_LANG_SPECIFIC (decl)
> > ! && DECL_MODULE_IMPORT_P (decl)))
> > ! {
> > ! auto kind = (TREE_CODE (decl) == NAMESPACE_DECL
> > ! && !DECL_NAMESPACE_ALIAS (decl)
> > ! ? depset::EK_NAMESPACE : depset::EK_DECL);
> > ! dep = dep_hash->add_dependency (decl, kind);
> > ! }
> > !
> > ! if (!dep)
> > ! {
> > ! /* Some internal entity of context. Do by value. */
> > ! decl_value (decl, NULL);
> > ! return false;
> > ! }
> > if (dep->get_entity_kind () == depset::EK_REDIRECT)
> > {
> >
> > where for a class-scope temporary we add a dependency for it, stream
> > it by reference, and then stream it by value separately, which seems
> > unnecessary.
> >
> > So if we decide to keep the create_temporary_var change, we probably
> > would want to unify this code path's handling of temporaries (i.e.
> > don't add_dependency a temporary regardless of its context).
> >
> > If we decide your partially revert the create_temporary_var change,
> > your patch LGTM.
>
> Streaming by value sounds right, but as noted an important difference
> between reference temps and others is DECL_NAME. Perhaps the code Patrick
> quotes could look at that as well as the context?
>
> Jason
>
With my patch we would no longer go through the code that Patrick quotes
for class-scope temporaries that I can see; we would instead first hit
the following code in 'tree_node':
if (DECL_P (t))
{
if (DECL_TEMPLATE_PARM_P (t))
{
tpl_parm_value (t);
goto done;
}
if (!DECL_CONTEXT (t))
{
/* There are a few cases of decls with no context. We'll write
these by value, but first assert they are cases we expect. */
gcc_checking_assert (ref == WK_normal);
switch (TREE_CODE (t))
{
default: gcc_unreachable ();
case LABEL_DECL:
/* CASE_LABEL_EXPRs contain uncontexted LABEL_DECLs. */
gcc_checking_assert (!DECL_NAME (t));
break;
case VAR_DECL:
/* AGGR_INIT_EXPRs cons up anonymous uncontexted VAR_DECLs. */
gcc_checking_assert (!DECL_NAME (t)
&& DECL_ARTIFICIAL (t));
break;
case PARM_DECL:
/* REQUIRES_EXPRs have a tree list of uncontexted
PARM_DECLS. It'd be nice if they had a
distinguishing flag to double check. */
break;
}
goto by_value;
}
}
skip_normal:
if (DECL_P (t) && !decl_node (t, ref))
goto done;
/* Otherwise by value */
by_value:
tree_value (t);
I think modifying what Patrick pointed out should only be necessary if
we maintain these nameless temporaries as having a class context; for
clarity, is that the direction you'd prefer me to go in to solve this?
Thanks,
Nathaniel
