Require canonical type comparison for typedefs again.

Hello,

I have reworked the patch to address your points. Please see my comments
below.

Jason Merrill <jason@redhat.com> writes:

> On 09/20/2010 12:29 PM, Dodji Seketeli wrote:
>>  #define TEMPLATE_TYPE_PARM_SIBLING_PARMS(NODE) \
>> -  (TREE_CHECK ((NODE), TEMPLATE_TYPE_PARM))->type.maxval
>> +  (TREE_CHECK3 ((NODE), TEMPLATE_TYPE_PARM,    \
>> +		TEMPLATE_TEMPLATE_PARM,	       \
>> +		BOUND_TEMPLATE_TEMPLATE_PARM))->type.maxval
>
> With your patch we aren't actually using this list anymore, only its
> length, so we might as well just store the length directly rather than
> the list.  That way, this:
>
>> +      /* If T1 belongs to a not-yet fully parsed template parameters
>> +	 list, let's assume it's different from T2 which belongs to an
>> +	 already fully parsed template template parameters list.  */
>> +      if ((TEMPLATE_TYPE_PARM_SIBLING_PARMS
>> +	   (TYPE_MAIN_VARIANT (t1)) != NULL_TREE)
>> +	  != (TEMPLATE_TYPE_PARM_SIBLING_PARMS
>> +	      (TYPE_MAIN_VARIANT (t1)) != NULL_TREE))
>> +	return false;
>> +
>> +      /* If T1 and T2 belong to template parm lists of different
>> +	 size, then can't be equal.  */
>> +      if ((TEMPLATE_TYPE_PARM_SIBLING_PARMS
>> +	   (TYPE_MAIN_VARIANT (t1)) != NULL_TREE)
>> +	  && (TEMPLATE_TYPE_PARM_SIBLING_PARMS
>> +	      (TYPE_MAIN_VARIANT (t2)) != NULL_TREE)
>> +	  && (TREE_VEC_LENGTH (INNERMOST_TEMPLATE_PARMS
>> +			       (TEMPLATE_TYPE_PARM_SIBLING_PARMS
>> +				(TYPE_MAIN_VARIANT (t1))))
>> +	      != TREE_VEC_LENGTH (INNERMOST_TEMPLATE_PARMS
>> +				  (TEMPLATE_TYPE_PARM_SIBLING_PARMS
>> +				   (TYPE_MAIN_VARIANT (t2))))))
>> +	return false;
>
> Can become a single simple comparison.  Which should go into
> comp_template_parms_position.
>

Nice. I have done that.

>> +/* Contains canonical template parameter types. The vector is indexed
>> +   by the TEMPLATE_TYPE_IDX of the template parameter. Each element is
>> +   a TREE_LIST, whose TREE_PURPOSE is a INT_CST tree representing a
>> +   total size of the template parameter list a given template
>> +   parameter would belong too, and whose TREE_VALUEs contain the
>> +   canonical template parameters of various types and levels,
>> +   belonging to a set of template parameters which size is
>> +   TREE_PURPOSE.  */
>>  static GTY(()) VEC(tree,gc) *canonical_template_parms;
>
> Since we're adding more elements to this table, maybe we should
> convert it to a hash table.  But that can be a follow-on patch.
>

Sure. I'll post a separate patch for it once we are done with this one.

>> +  if (total_num_parms == 0)
>> +    return NULL_TREE;
>
> Since you're already treating parms from a list of unknown length as
> distinct from parms from a list of known length, we can give them a
> canonical type, too.
>

Doing that doesn't seem to work. The problem is not with the comparison
of type template parms but rather with the comparison of dependent types
created during the template parms parsing.

If type template parms don't require structural comparison then a type A
(created during template parms parsing) dependent on those parms will
have a canonical type. The fixup then creates a type A' that is
structurally equivalent to A, but has a different canonical type. Later,
possible calls to lookup_template_class could then compare A and A' and
the sanity constraints activated by ENABLE_CHECKING in comptypes would
be violated because two structurally equivalent types have different
canonical types.

We don't have the issue in comptypes if A doesn't have a canonical type
to begin with.

>> +      if (TREE_CODE (parm) == TYPE_DECL
>> +	     || TREE_CODE (parm) == TEMPLATE_DECL)
>> +	{
>> +	  /* Compute the canonical type of type template
>> +	     parameters and their variants.  */
>
> Hmm, don't we need to handle substituting type parameters into
> template parameters?
>
> template <class T, template <T> class U> struct A { };
>
> It should work to do a single loop over the template args, doing any
> substitution and then replacing them with their canonical variants as
> appropriate.  Since a particular parameter can only refer to previous
> ones, we can't run into ordering issues.
>

Good catch.

I have done this but it made me scratch head for a little while because
tsubsting into the parms of a template template parm wasn't as straight
forward as I thought it would be.

The issue where the followings.

* Template parm level reduction.

When at least one of the parms is itself a template [like in
testsuite/g++.old-deja/g++.pt/nttp1.C] then at some point we end up in
tsubst (at least) with an argument set that has a depth that is less
than the depth of the parm of the template template parm we are
susbtituting into.

My understanding is that the tsubsting code then thinks that we are
doing the substitution for the purpose of instantiation and then reduces
the level of the resulting parms.

In this current case though, we are not doing the substitution for the
purpose of instantiation; we want the levels of all the resulting
tsubsted parms to remain unchanged; we want an "in place substitution"
so to speak.

So I introduced a new tf_no_tpl_reduce (for "no template parm level
reduce") flag in enum tsubst_flags and changed tsubst and
tsubst_template_parms to avoid doing level reduction when passed this
flag.]

Incidentally I noticed that the tf_no_class_instantiation is not used by
any client code anymore. Would you accept a patchlet to remove it?

* I had to use use_template_parms in make_typename_type

I know I have to be careful each time I use use_template_parms instead
of dependent_type_p so I'll give some context.

Consider this example distilled from testsuite/g++.dg/template/ttp25.C:

~=~
template<typename T> struct metafun { typedef T type; };

template<typename T, template<typename metafun<T>::type> class C> //#0
void f4(T, C<5>);

template<int N> struct X {};
void g() {
  f4(5, X<5>()); // OK.
}
~=~

Consider when we build the set of overload candidate functions for
f4(5, X<5>()). add_template_candidate_real at some point tries to deduce the
template argument to C.

During unification of C<5> with X<5> the argument 5 of X is converted
into the type of the parameter of C, i.e 5 is converted into typename
metafun<T>::type. convert_template_argument then tries to substitute
into typename metafun<T>::type. This should just create another
equivalent typename becase metafun<T> is dependent and thus non
complete. But when tsubst calls make_typename_type, the later fails to
detect that metafun<T> is dependent [because processing_template_decl is
0] and goes crazy. As tsubst uses use_template_parms to detect that
metafun<T> is dependent before calling make_typename_type, I thought I'd
use the same in the later.

Interestingly, the reason why this wasn't happening before is, in #0,
when lookup_class_template is called to build the type of metafun<T>,
that template-id is considered as naming template<class T> struct
metafun; so its canonical type is set to the TREE_TYPE of the
TEMPLATE_DECL of metafun. That type is complete. So later when
make_typename_type is called during the overload resolution of
f4(5, X<5>()), the "if (!dependent_scope_p (context))" succeeds and the
call to t = lookup_field (context, name, 2, /*want_type=*/true);
returns the field "type".

Tested for the c++ languages against trunk on
x86_64-unknown-linux-gnu. I am going launch a test for the other
languages right away.

Require canonical type comparison for typedefs again.

Commit Message

Comments

Patch