On Thu, Oct 24, 2024 at 12:05:18PM -0400, Jason Merrill wrote:
> On 10/24/24 3:25 AM, Nathaniel Shead wrote:
> > I wasn't sure whether I should include the ambiguity checking logic from
> > process_partial_specialization; we don't do this anywhere else in the
> > modules handling code that I could see so I left it out for now.
>
> The relevant bit in the standard seems to be
> https://eel.is/c++draft/temp#spec.partial.general-1
> "A partial specialization shall be reachable from any use of a template
> specialization that would make use of the partial specialization as the
> result of an implicit or explicit instantiation; no diagnostic is required."
>
> So we aren't required to diagnose an imported partial spec after an
> instantiation. It's not entirely clear whether the ambiguity checking in
> https://eel.is/c++draft/temp#spec.partial.match-1.2 qualifies as "make use",
> but that seems a natural interpretation.
>
> So checking these on import seems optional, but could also be helpful in
> finding subtle bugs.
>
Makes sense. I might add this as a follow-up patch then, since if we're
doing this here it would make sense to do in add_mergeable_specialization
as well now that I think about it.
> > I could also rework process_partial_specialization to call the new
> > create_mergeable_partial_spec function if you prefer, to reduce code
> > duplication, though the early exit for VAR_DECL and the call to
> > associate_classtype_constraints that relies on global state complicated
> > it enough I didn't bother for this patch.
>
> That sounds desirable, if we need to create the mergeable partial spec at
> all.
>
> > Finally, I'm not entirely sure whether the choice of DECL_TEMPLATE_PARMS
> > that I'm providing is correct, especially considering this hunk:
> >
> > /* Give template template parms a DECL_CONTEXT of the template
> > for which they are a parameter. */
> > for (i = 0; i < ntparms; ++i)
> > {
> > tree parm = TREE_VALUE (TREE_VEC_ELT (inner_parms, i));
> > if (TREE_CODE (parm) == TEMPLATE_DECL)
> > DECL_CONTEXT (parm) = tmpl;
> > }
> >
> > I haven't been able to produce any testcase that relies on this context
> > setting to cause a failure, though, and there doesn't seem to be one in
> > the testsuite for the matching hunk in process_partial_specialization.
> > Any hints on where this might be needed?
>
> I believe that's for the benefit of coerce_template_args_for_ttp.
>
OK thanks, I'll do some exploring to see if I can cause a failure here
then.
> > In some cases, when we go to import a partial specialisation there might
> > already be an incomplete implicit instantiation in the specialisation
> > table. This causes ICEs described in the linked PR as we now have two
> > separate matching specialisations for this same arguments.
> >
> > This issue doesn't appear to happen for variable templates as it doesn't
> > appear that we can create incomplete specialisations in the same way.
> >
> > This patch attempts to solve the issue by retrofitting the existing
> > implicit instantiation as a partial specialisation (similarly to how
> > maybe_process_partial_specialization operates) and then relying on the
> > existing merging logic to fill in the definition of the type. This
> > works because for types almost all relevant details are streamed and
> > merged in 'read_class_def'.
>
> I find it surprising that we would need to build a partial specialization to
> merge with, when in a single TU we have no trouble merging a partial
> specialization with an existing implicit instantiation. Why can't we do
> that in this case as well?
>
As far as I can tell, this is the normal behaviour for declarations;
when we do
template <typename T> struct S;
template <typename T> struct S<T*>
^~~~~
This registers an implicit instantiation in the specialisation table
already; the parser then calls 'maybe_process_partial_specialization'
which overwrites the implicit instantiation as a partial specialization
instead, see:
/* This is for ordinary explicit specialization and partial
specialization of a template class such as:
template <> class C<int>;
or:
template <class T> class C<T*>;
Make sure that `C<int>' and `C<T*>' are implicit instantiations. */
if (maybe_new_partial_specialization (type))
{
if (!check_specialization_namespace (CLASSTYPE_TI_TEMPLATE (type))
&& !at_namespace_scope_p ())
return error_mark_node;
SET_CLASSTYPE_TEMPLATE_SPECIALIZATION (type);
DECL_SOURCE_LOCATION (TYPE_MAIN_DECL (type)) = input_location;
if (processing_template_decl)
{
tree decl = push_template_decl (TYPE_MAIN_DECL (type));
if (decl == error_mark_node)
return error_mark_node;
return TREE_TYPE (decl);
}
}
The difference with modules is that rather than being able to overwrite
the existing instantiation with a definition in-place, we instead
currently get given a new tree with the entire template/decl/type of a
partial specialisation, but there is an existing template/decl/type of
an implicit instantiation in the specialisation table (and pointed to by
other by decls, in the case of the PR via the return type of a function).
So we need to somehow update in-place the existing type to be the type
of the partial specialisation so that we don't break any decls that
refer to it already. My approach here is to attempt to copy the
relevant parts of what maybe_process_partial_specialization is doing so
we can pretend that we're merging an existing partial specialisation
(and use the rest of the existing logic) rather than doing part of the
in-place update of the implicit instantiation from within
add_mergeable_specialization, and then again once we get to
read_class_def.
Happy to try another route if you think there's something I've missed
here though.
> > As an optimisation, we also mark the newly retrofitted specialisation as
> > imported, since there isn't any useful information on the declaration
> > that will be provided from this TU anyway, and so we don't need to write
> > the decl into our own BMI but can instead just seed it as an import.
>
> Agreed.
>
> Jason
>
