<div class="gmail_quote">On Mon, Apr 23, 2012 at 2:24 PM, Richard Smith <span dir="ltr"><<a href="mailto:richard@metafoo.co.uk">richard@metafoo.co.uk</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="gmail_quote">On Thu, Apr 19, 2012 at 5:40 AM, Daniel Jasper <span dir="ltr"><<a href="mailto:djasper@google.com" target="_blank">djasper@google.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">


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()?</blockquote>


<div><br></div><div>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.</div>


<div><br></div><div>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.</div>

<div><br></div><div>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:</div>

<div><br></div><div>// I think we don't visit the instantiations of A</div><div>template<typename T> struct A;</div><div>A<int> *p;</div><div><br></div><div>template<typename T> struct B {</div><div>

  // With your patch, I think we do visit the instantiations of B<int>::C</div><div>  template<typename U> struct C;</div><div>};</div><div>B<int>::C<int> *q;</div><div><br></div><div>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?</div>
</div></blockquote><div><br></div><div>I've done some more digging into this.</div><div><br></div><div>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.</div>
<div><br></div><div>Separately from that, we have the question of whether we should be visiting such declarations. I think we probably should.</div><div><br></div><div>-- Richard</div></div>