Commit Message

Hello Paolo, Jason,
I am replying to both of your messages in below.
Paolo Carlini <pcarlini@gmail.com> writes:
[...]
>> diff --git a/gcc/testsuite/g++.dg/template/typedef36.C b/gcc/testsuite/g++.dg/template/typedef36.C>> new file mode 100644>> index 0000000..f6155c1>> --- /dev/null>> +++ b/gcc/testsuite/g++.dg/template/typedef36.C>> @@ -0,0 +1,23 @@>> +// Origin: PR c++/45606>> +// { dg-do compile }>> +>> +#include <list>
[...]
>> Once again, if, thanks to the analysis you did to fix the problem, you> are able to quickly figure out a shorter testcase, not including the> entire <list>, I think it would be good.
You are right. Sorry for not having done it before. It is now done in
the udpated patch below.
Jason Merrill <jason@redhat.com> writes:
[...]
>> -/* 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_VALUEs contain the canonical template>> - parameters of various types and levels. */>> +/* 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. */>> We don't really need to change the representation this way, do we? We> can just let structural_comptypes verify that the total parms #> matches. Same with the changes to canonical_type_parameter.
Done.
>>> + if (TREE_PURPOSE (cur))>> + {>> + /* So the current template type parameter has a default>> + argument. Substitute the so far fixed-up template>> + parms into it. */>> + tree default_arg = TREE_PURPOSE (cur);>> +>> + if (TYPE_P (default_arg)>> + && !dependent_type_p (default_arg))>> + continue;>> +>> + arglist = current_template_args ();>> Why do we need to get current_template_args() again here?>
I wonder why myself. Thanks for catching this. I removed it.
>> + ++cp_unevaluated_operand;>> + TREE_PURPOSE (cur) =>> + tsubst_expr (default_arg, arglist,>> + tf_none, default_arg, false);>> + --cp_unevaluated_operand;>> tsubst (default_arg, arglist, tf_warning_or_error, parm);>> And don't mess with cp_unevaluated_operand.>
Done.
>> + /* Now we need to substitute the template parm types that>> + have been fixed up so far into the non-type template>> + parms of this template template parm. E.g, consider this:>> This needs to be recursive, in case the template template parm has its> own template template parms.>> template <class T, template <template <class U = T> class V> class W>> class A;
Oops, done now.
>> + ++cp_unevaluated_operand;>> + substed_parm = tsubst_template_parm (parameter, targs, tf_none);>> + --cp_unevaluated_operand;>> Still shouldn't be messing with cp_unevaluated_operand. And we should> use tf_warning_or_error.
Done.
>> Also, don't we need to handle default template template arguments?>
I am not sure about this but can a default template template argument
depend on the template parms of the current template being declared?
e.g, in:
template<class T>
struct C
{
};
template<class U,
template<class V> class W = C>
struct S
{
};
Here, the template parms of C haven't been created in the template parm
list of S. Furthermore, couldn't tsubsting U into C would wrongly
remplace the template parm T of C with U? I said wrongly because now
with this patch T and U are different types.
> template <template <class T> class A, template <class T> class B = A>> class C;>>> + /* Add this fixed up PARM to the template parms we've fixed>> + up so far and use that to substitute the fixed-up>> + template parms into the type of PARM. */>> + TREE_VEC_ELT (fixedup_args, i) = template_parm_to_arg (cur);>> + TREE_TYPE (parm) = tsubst (TREE_TYPE (parm), arglist,>> + tf_none, NULL_TREE);>> Don't we need to set the type of pushed_decl, too?
Ah, it is done by the call to fixup_template_parm_index right before the
snippet of code you quoted. as that function takes care of updating all
references to template type parms from the index. I have added a comment
for that.
>>> + TREE_VALUE (cur) = parm;>> But we don't need to set this, as parm hasn't changed.>
Sigh. True. Removed.
>> +extern void set_template_parms_num_siblings (tree, int);>> I don't think we want this function. Instead, add a siblings parm to> build_template_parm_index.>
Done.
>> @@ -17789,7 +18218,8 @@ type_dependent_expression_p (tree expression)>> if (!processing_template_decl)>> return false;>>>> - if (expression == error_mark_node)>> + if (expression == error_mark_node>> + || expression == NULL_TREE)>> What made this necessary? I would think this should be caught in the> caller.
This is actually not necessary. I think I tentatively added this while
looking for why I was breaking so many regression tests at some point
and forgot to remove it. I have removed it from the patch.
>>> + /* If T1 and T2 belong to template parm lists of different size,>> + let's assume they are different. */>> + if (get_template_parms_num_siblings (TYPE_MAIN_VARIANT (t1))>> + != get_template_parms_num_siblings (TYPE_MAIN_VARIANT (t2)))>> return false;>>> + /* Then compare their relative position. */>> + if (TEMPLATE_TYPE_IDX (t1) != TEMPLATE_TYPE_IDX (t2)>> + || TEMPLATE_TYPE_LEVEL (t1) != TEMPLATE_TYPE_LEVEL (t2)>> + || (TEMPLATE_TYPE_PARAMETER_PACK (t1)>> + != TEMPLATE_TYPE_PARAMETER_PACK (t2)))>> All of these are looking at the TEMPLATE_PARM_INDEX, so we might as> well pull it out and operate on it directly with TEMPLATE_PARM_*.>
Done. With this change we don't need get_template_parms_num_siblings
anymore either so I have removed it too.
> You need more testcases.
I have added some more.
I fully bootstrapped and tested the patch on x86_64-unknown-linux-gnu.
Thanks.

Comments

On 10/15/2010 11:44 AM, Dodji Seketeli wrote:
>> Also, don't we need to handle default template template arguments?>> I am not sure about this but can a default template template argument> depend on the template parms of the current template being declared?
Well, yes, as in the example I gave with that question:
>> template<template<class T> class A, template<class T> class B = A>>> class C;
> template<class T>> struct C> {> };>> template<class U,> template<class V> class W = C>> struct S> {> };>> Here, the template parms of C haven't been created in the template parm> list of S. Furthermore, couldn't tsubsting U into C would wrongly> remplace the template parm T of C with U? I said wrongly because now> with this patch T and U are different types.
Right, we don't want to modify C. Doesn't tsubst_template_arg do the
right thing? It looks to me like it ends up calling tsubst_copy, which
should return C unchanged.
Actually, I guess you can use tsubst_template_arg for all three
varieties of template parameter, rather than handle each one specifically.
Jason