[cfe-commits] Fix for RecursiveASTMatcher not traversing nested template instatiations

[Richard Smith]

On Mon, Apr 23, 2012 at 2:24 PM, Richard Smith <richard at metafoo.co.uk>wrote:

> On Thu, Apr 19, 2012 at 5:40 AM, Daniel Jasper <djasper at google.com> wrote:
>
>> Please find a new version of the patch attached. This also fixes the same
>> problem in nested function templates and includes corresponding tests. I am
>> still not sure whether this is just a workaround for an underlying problem:
>> Shouldn't the Decl fulfill isThisDeclarationADefinition()?
>
>
> When a template is a member of another template, instantiating the outer
> template doesn't cause an instantiation of the inner template. In your
> added testcase, we instantiate X<A>::Y<A> directly from X::Y, and never
> instantiate X<A>::Y. Since isThisDeclarationADefinition is used to
> determine whether we have an available definition (rather than a definition
> which could be instantiated), it has to return false for X<A>::Y.
>
> I don't think we have any convenient way of asking the question which this
> code wants to ask (which is, "is this the declaration with which the
> instantiations of this template should be associated?"), but what you have
> there looks reasonable.
>
> I have one concern: it looks like we don't currently visit instantiations
> of template classes which are declared, but not defined. With your change,
> I think we will visit those instantiations for member templates of class
> templates:
>
> // I think we don't visit the instantiations of A
> template<typename T> struct A;
> A<int> *p;
>
> template<typename T> struct B {
>   // With your patch, I think we do visit the instantiations of B<int>::C
>   template<typename U> struct C;
> };
> B<int>::C<int> *q;
>
> I'm not sure to what extent this matters, but it would be good to be
> consistent. Perhaps we should follow the getInstantiatedFromMemberTemplate
> chain back to the original template, and check whether that declaration is
> a definition?
>

I've done some more digging into this.

RecursiveASTVisitor::TraverseClassInstantiations completely ignores
templates for which we have instantiated the declaration but not the
definition, so your patch does not introduce the problem I was concerned
about. So: LGTM.

Separately from that, we have the question of whether we should be visiting
such declarations. I think we probably should.

-- Richard
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120424/74f86d5a/attachment.html>