<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Thu, Feb 5, 2015 at 4:45 AM, Alexey Bataev <span dir="ltr"><<a href="mailto:a.bataev@hotmail.com" target="_blank">a.bataev@hotmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Richard, Reid,<br>
Thanks for the review!<br>
<span class=""><br>
<br>
================<br>
Comment at: lib/AST/Type.cpp:1894-1900<br>
@@ -1893,5 +1893,9 @@<br>
<br>
TagDecl *TagType::getDecl() const {<br>
return getInterestingTagDecl(decl);<br>
}<br>
<br>
+TagDecl *TagType::getDecl() {<br>
+ return getInterestingTagDecl(decl);<br>
+}<br>
+<br>
----------------<br>
</span><span class="">rsmith wrote:<br>
> What's the purpose of this change? We'll call the `const` version for a non-const object without this, which will do the same thing.<br>
</span>Removed.<br>
<span class=""><br>
================<br>
Comment at: lib/Sema/SemaDecl.cpp:132<br>
@@ +131,3 @@<br>
+namespace {<br>
+enum BasesLookupResult { NotFound, FoundNotType, FoundType };<br>
+} // namespace<br>
----------------<br>
</span><span class="">rsmith wrote:<br>
> rnk wrote:<br>
> > "FoundNonType" would be closer to the standardese terminology of "non-type template parameter"<br>
> This should either be an enum class or should use a prefix for its names; these identifiers seem too general to drop into global scope here.<br>
</span>Turned enum into enum class and renamed FoundNotType to FoundNonTypeTemplateParam<br>
<span class=""><br>
================<br>
Comment at: lib/Sema/SemaDecl.cpp:139<br>
@@ +138,3 @@<br>
+/// type decl, \a FoundType if only type decls are found.<br>
+static BasesLookupResult lookupInBases(Sema &S, const IdentifierInfo &II,<br>
+ SourceLocation NameLoc,<br>
----------------<br>
</span><span class="">rsmith wrote:<br>
> The name of this function should mention something about dependent bases, since that's what's interesting about it and how it differs from normal name lookup.<br>
</span>Renamed to lookupUnqualifiedTypeNameInDependentBase<br>
<span class=""><br>
================<br>
Comment at: lib/Sema/SemaDecl.cpp:153<br>
@@ +152,3 @@<br>
+ /*LookupKind=*/Sema::LookupAnyName);<br>
+ if (!S.LookupQualifiedName(LR, DC) &&<br>
+ LR.getResultKind() == LookupResult::NotFound)<br>
----------------<br>
</span><span class="">rsmith wrote:<br>
> You're going to lengths to ensure we look up into dependent bases of dependent bases, but you don't support finding names in dependent bases of non-dependent bases of dependent bases: we'll use normal name lookup here in that case. Maybe remove this whole case and do the same thing you do below: look up into the RecordDecl you get here and then recurse to its base classes.<br>
</span>Hmm, maybe I'm missing something, but is it possible at all? How non-dependent class may have dependent base?</blockquote><div><br></div><div>The base class may be part of the current instantiation:</div><div><br></div><div>template<typename T> struct A { typedef int type; };</div><div>template<typename T> struct B : A<T> {</div><div> struct C;</div><div>};</div><div>template<typename T> struct B<T>::C : B {</div><div> void f() {</div><div> type x;</div><div> }</div><div>};</div><div><br></div><div>Here, B<T> is a non-dependent base class of B<T>::C, because it is part of the same "current instantiation" (that is, whenever you're instantiating that definition of B<T>::C, you must also have an instantiation of the primary template of B<T>).</div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="">
================<br>
Comment at: lib/Sema/SemaDecl.cpp:159-160<br>
@@ +158,4 @@<br>
</span><span class="">+ FoundTypeDecl = FoundType;<br>
+ }<br>
</span><span class="">+ } else if (auto *TST =<br>
+ Base.getType()->getAs<TemplateSpecializationType>()) {<br>
----------------<br>
</span><span class="">rnk wrote:<br>
> You can reduce the indentation here with continue.<br>
</span>Ok<br>
<span class=""><br>
================<br>
Comment at: lib/Sema/SemaDecl.cpp:173-187<br>
@@ +172,17 @@<br>
+ continue;<br>
+ switch (lookupInBases(S, II, NameLoc, BasePrimaryTemplate)) {<br>
+ case FoundNotType:<br>
+ return FoundNotType;<br>
+ case FoundType:<br>
+ FoundTypeDecl = FoundType;<br>
+ break;<br>
+ case NotFound:<br>
+ break;<br>
+ }<br>
+ for (NamedDecl *ND : BasePrimaryTemplate->lookup(&II)) {<br>
+ if (!isa<TypeDecl>(ND))<br>
+ return FoundNotType;<br>
+ FoundTypeDecl = FoundType;<br>
+ }<br>
+ }<br>
+ }<br>
----------------<br>
</span><span class="">rsmith wrote:<br>
> Switch these two over: first look up in the base class itself, and then look up in its base classes if you find nothing. Otherwise you'll get the wrong result if the name is a type in one place and a non-type in the other.<br>
</span>Ok, got it.<br>
<span class=""><br>
================<br>
Comment at: lib/Sema/SemaDecl.cpp:196-202<br>
@@ -133,13 +195,9 @@<br>
SourceLocation NameLoc) {<br>
// Find the first parent class template context, if any.<br>
- // FIXME: Perform the lookup in all enclosing class templates.<br>
const CXXRecordDecl *RD = nullptr;<br>
for (DeclContext *DC = S.CurContext; DC; DC = DC->getParent()) {<br>
RD = dyn_cast<CXXRecordDecl>(DC);<br>
if (RD && RD->getDescribedClassTemplate())<br>
break;<br>
}<br>
// Look for type decls in dependent base classes that have known primary<br>
----------------<br>
</span><span class="">rsmith wrote:<br>
> If there are multiple such base classes, we should ideally search outer ones if we find nothing in the innermost one, to match normal unqualified lookup.<br>
</span>Agree, will be fixed<br>
<span class=""><br>
================<br>
Comment at: test/SemaTemplate/ms-lookup-template-base-classes.cpp:411<br>
@@ -392,2 +410,3 @@<br>
+C<int> c; // expected-note {{in instantiation of template class 'type_in_base_of_multiple_dependent_bases::C<int>' requested here}}<br>
}<br>
<br>
----------------<br>
</span><span class="">rnk wrote:<br>
> Can you add a test for the case where we look through non-dependent bases? So far as I can tell, these are all dependent.<br>
</span>Test in 'namespace type_in_base_of_dependent_base' checks this already. But I'll add another one.<br>
<div class="HOEnZb"><div class="h5"><br>
<a href="http://reviews.llvm.org/D7173" target="_blank">http://reviews.llvm.org/D7173</a><br>
<br>
EMAIL PREFERENCES<br>
<a href="http://reviews.llvm.org/settings/panel/emailpreferences/" target="_blank">http://reviews.llvm.org/settings/panel/emailpreferences/</a><br>
<br>
<br>
</div></div></blockquote></div><br></div></div>