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

[Bataev, Alexey]

Richard, I got it, thanks.

Best regards,
Alexey Bataev
=============
Software Engineer
Intel Compiler Team

06.02.2015 0:54, Richard Smith пишет:
> On Thu, Feb 5, 2015 at 4:45 AM, Alexey Bataev <a.bataev at hotmail.com 
> <mailto:a.bataev at hotmail.com>> wrote:
>
>     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?
>
>
> The base class may be part of the current instantiation:
>
> template<typename T> struct A { typedef int type; };
> template<typename T> struct B : A<T> {
>   struct C;
> };
> template<typename T> struct B<T>::C : B {
>   void f() {
>     type x;
>   }
> };
>
> 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>).
>
>     ================
>     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/
>
>
>