[PATCH] [MSVC] Improved lookup into dependent/non-dependent bases of dependent class

[Alexey Bataev]

Richard, Reid,
Thanks for the review!


================
Comment at: lib/AST/Type.cpp:1894-1900
@@ -1893,5 +1893,9 @@
 
 TagDecl *TagType::getDecl() const {
   return getInterestingTagDecl(decl);
 }
 
+TagDecl *TagType::getDecl() {
+  return getInterestingTagDecl(decl);
+}
+
----------------
rsmith wrote:
> 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.
Removed.

================
Comment at: lib/Sema/SemaDecl.cpp:132
@@ +131,3 @@
+namespace {
+enum BasesLookupResult { NotFound, FoundNotType, FoundType };
+} // namespace
----------------
rsmith wrote:
> rnk wrote:
> > "FoundNonType" would be closer to the standardese terminology of "non-type template parameter"
> 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.
Turned enum into enum class and renamed FoundNotType to FoundNonTypeTemplateParam

================
Comment at: lib/Sema/SemaDecl.cpp:139
@@ +138,3 @@
+/// type decl, \a FoundType if only type decls are found.
+static BasesLookupResult lookupInBases(Sema &S, const IdentifierInfo &II,
+                                       SourceLocation NameLoc,
----------------
rsmith wrote:
> 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.
Renamed to lookupUnqualifiedTypeNameInDependentBase

================
Comment at: lib/Sema/SemaDecl.cpp:153
@@ +152,3 @@
+                        /*LookupKind=*/Sema::LookupAnyName);
+        if (!S.LookupQualifiedName(LR, DC) &&
+            LR.getResultKind() == LookupResult::NotFound)
----------------
rsmith wrote:
> 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.
Hmm, maybe I'm missing something, but is it possible at all? How non-dependent class may have dependent base?

================
Comment at: lib/Sema/SemaDecl.cpp:159-160
@@ +158,4 @@
+        FoundTypeDecl = FoundType;
+      }
+    } else if (auto *TST =
+                   Base.getType()->getAs<TemplateSpecializationType>()) {
----------------
rnk wrote:
> You can reduce the indentation here with continue.
Ok

================
Comment at: lib/Sema/SemaDecl.cpp:173-187
@@ +172,17 @@
+        continue;
+      switch (lookupInBases(S, II, NameLoc, BasePrimaryTemplate)) {
+      case FoundNotType:
+        return FoundNotType;
+      case FoundType:
+        FoundTypeDecl = FoundType;
+        break;
+      case NotFound:
+        break;
+      }
+      for (NamedDecl *ND : BasePrimaryTemplate->lookup(&II)) {
+        if (!isa<TypeDecl>(ND))
+          return FoundNotType;
+        FoundTypeDecl = FoundType;
+      }
+    }
+  }
----------------
rsmith wrote:
> 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.
Ok, got it.

================
Comment at: lib/Sema/SemaDecl.cpp:196-202
@@ -133,13 +195,9 @@
                                                       SourceLocation NameLoc) {
   // Find the first parent class template context, if any.
-  // FIXME: Perform the lookup in all enclosing class templates.
   const CXXRecordDecl *RD = nullptr;
   for (DeclContext *DC = S.CurContext; DC; DC = DC->getParent()) {
     RD = dyn_cast<CXXRecordDecl>(DC);
     if (RD && RD->getDescribedClassTemplate())
       break;
   }
   // Look for type decls in dependent base classes that have known primary
----------------
rsmith wrote:
> 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.
Agree, will be fixed

================
Comment at: test/SemaTemplate/ms-lookup-template-base-classes.cpp:411
@@ -392,2 +410,3 @@
+C<int> c; // expected-note {{in instantiation of template class 'type_in_base_of_multiple_dependent_bases::C<int>' requested here}}
 }
 
----------------
rnk wrote:
> 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.
Test in 'namespace type_in_base_of_dependent_base' checks this already. But I'll add another one.

http://reviews.llvm.org/D7173

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/