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

[Richard Smith]

On Thu, Feb 5, 2015 at 4:45 AM, Alexey Bataev <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/
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20150205/5cc142da/attachment.html>