[cfe-commits] [Patch][Review request] RecursiveASTVisitor can now traverse template specializations

[John McCall]

On Sep 1, 2010, at 11:10 AM, Benoit Belley wrote:
> Le 2010-08-30 à 21:37, John McCall a écrit :
>> On Aug 30, 2010, at 6:08 PM, John McCall wrote:
>>> On Aug 30, 2010, at 5:33 PM, Benoit Belley wrote:
>>>> I think that I’ll need a bit more explanation here. The infinite recursion occurs while traversing the instantiations of the class template definition and I do want to traverse these instantiations. That’s the whole point of my proposed change.
>>> 
>>> Yes, I understood that.  My point is that you're visiting the instantiations regardless of whether the current template declaration you're visiting is a redeclaration or not.  This creates two problems:  first, you end up visiting the instantiations multiple times if the template is redeclared; and second, if the template is redeclared within itself (which can only be done with friend declarations), you end up visiting it recursively, which blows up the stack.  The solution is to only visit implicit instantiations when you visit a pattern definition.
>>> 
>>> template <class T> class A;  // <- don't visit any instantiations here
>>> class B { template <class T> friend class A; };  // <- or here
>>> template <class T> class A { ... }; // <- okay to visit implicit instantiations here
>> 
>> A few more notes.
>> 
>> First, note that my proposal means that you won't be visiting implicit instantiations for undefined templates.  I'm assuming that's okay;  otherwise, you'll need to use different logic (I think visiting instantiations for the first declaration should be sufficient).
>> 
> 
> I’m also ok with not visiting implicit instantiations for undefined templates.

Okay, that makes sense.

>> Second, have you considered what you want to do with incomplete instantiations?  These arise when a template specialization is written but not required to be a complete type.  My thought is that these should be ignored, but you may feel differently.
>> 
> 
> No, I have not considered them up to now. By incomplete instantiation, do you mean the following:
> i.e. where I have declared that a specialization for A<int> exists even though I have not defined it.

No, I mean something like this:
  template <class T> struct A { T foo; };
  int bar(A<T> &a) { return 0; }
i.e. where a specialization of A was used but not required to be a complete type and so template instantiation was never done.

> If so, my RecursiveASTVisitor to behave like expected, i.e. the instantiation for A<int> is never visited.
> 
>> Third, you need to decide what you want to do with instantiations of partial specializations.  A significant issue here is that it's possible to have complete instantiations of a template without having a definition for the primary template.  I think the most appropriate general solution is to visit instantiations whenever you reach their pattern;  for example:
>> template <class T> class A { ... }; // visit A<bool>, A<int> here
>> template <class T> class A<T*> { ... }; // visit A<char*>, A<int*> here
>> 
> 
> Ok, I fixed this using:
> 
>    CXXRecordDecl* TempDecl = D->getTemplatedDecl();
>    TRY_TO(TraverseDecl(TempDecl));
>    TRY_TO(TraverseTemplateParameterListHelper(D->getTemplateParameters()));
>    // Explicit class intantiations and specializations will be
>    // traversed from the context of their declaration. There
>    // is therefore no need to traverse them for here.
>    //
>    // In addition, we only traverse the class instantiations when the
>    // class template is class template definition or the canonical
>    // declaration when no definition of the class exists.
>    if (TempDecl) {
>      CXXRecordDecl* DefDecl  = TempDecl->getDefinition();
>      CXXRecordDecl* CanoDecl = TempDecl->getCanonicalDecl();
>      if (TempDecl == DefDecl || (!DefDecl && TempDecl == CanoDecl)) {
>        TRY_TO(TraverseImplicitClassInstantiations(D));
>      }
>    }
> 
> Note that the behavior ends-up being slightly different from what you suggested. The instantiations are always traverse in the scope of a class declaration ClassTemplateDecl (either the definition or the canonical declaration when no definition exists). Instantiations of partial specialization are not traverse in the scope of the ClassTemplatePartialSpecializationDeclaration.

I guess this is fine, although please try to avoid doing the getDefinition() / getCanonicalDecl() work if we're not visiting instantiations.  If someone cares about exact ordering of visitation, we can change it then and it shouldn't hurt you.

>> Finally, it looks like there's an existing bug (with a FIXME) where we're not visiting the bodies of explicit specializations.  If you could fix TraverseClassTemplateSpecializationDecl to visit the record when either your flag is set *or* it's an explicit specialization, that would be good.
>> 
> 
> Sure.

Thanks.

John.