[PATCH] [MSVC] Better lookup into dependent bases for unspecified declaration (also fixes http://llvm.org/PR22066)
Richard Smith
richard at metafoo.co.uk
Tue Mar 17 17:23:56 PDT 2015
================
Comment at: include/clang/Sema/Sema.h:3500-3518
@@ -3499,1 +3499,21 @@
+ /// \brief Checks if currently looking for symbols in a class with dependent
+ /// bases. Used in Microsoft Compatibility mode for better compatibility
+ /// during lookup into classes with dependent bases.
+ /// \param LastChanceToRecover true if no need to check for decl existence in
+ /// base dependent classes, false if still need to check.
+ /// \param SS If not nullptr and not empty the base class is calculated from
+ /// this scope. If not nullptr and empty and \a ObjectType is empty then it is
+ /// calculated in this method from the current context.
+ /// \param ObjectType If specified the base class is calculated from this
+ /// type.
+ /// \param II Used for diagnostic only. If \a SS is not nullptr and empty and
+ /// \a ObjectType is empty and \a is not nullptr \a SS is calculated and a
+ /// diagnostic is emitted.
+ /// \return true if currently trying to lookup in class with the dependent
+ /// base, false otherwise.
+ bool isInClassWithAnyDependentBase(SourceLocation Loc,
+ bool IsLastChanceToRecover,
+ CXXScopeSpec *SS, QualType ObjectType,
+ const IdentifierInfo *II);
+
----------------
>From reading this alone, I have very little idea what this function does.
================
Comment at: lib/Sema/SemaDecl.cpp:137
@@ -133,3 +136,3 @@
NotFound,
- FoundNonType,
- FoundType
+ /// \brief If found a member, but its type does not match the expected one.
+ FoundNotSpecifiedKind,
----------------
I don't think you mean 'type' here. Also remove the 'If'.
================
Comment at: lib/Sema/SemaDecl.cpp:139
@@ +138,3 @@
+ FoundNotSpecifiedKind,
+ /// \brief There are no members is found, but class has a base of class
+ /// TemplateTypeParmType and we are allowed to suppose that the specified
----------------
Remove 'is'.
================
Comment at: lib/Sema/SemaDecl.cpp:140
@@ +139,3 @@
+ /// \brief There are no members is found, but class has a base of class
+ /// TemplateTypeParmType and we are allowed to suppose that the specified
+ /// identifier may be a member of this base.
----------------
Why only of that type? Why not any dependent type that we can't heuristically look into?
================
Comment at: lib/Sema/SemaDecl.cpp:143
@@ +142,3 @@
+ NotFoundWithUnknownBase,
+ /// \brief Found a member of the expected type.
+ FoundSpecifiedKind,
----------------
Again, 'type' seems wrong here. 'kind' perhaps?
================
Comment at: lib/Sema/SemaDecl.cpp:148-149
@@ -138,3 +147,4 @@
-/// \brief Tries to perform unqualified lookup of the type decls in bases for
+/// \brief Tries to perform unqualified lookup of the decls in bases for
/// dependent class.
+/// \return \a NotFound if no any decls is found, \a FoundNotSpecifiedKind if
----------------
How about something like "Attempt to determine what lookup of a name would find in an instantiation of a dependent class. For a dependent base class, we make a heuristic guess as to the template from which the base class will be instantiated."
================
Comment at: lib/Sema/SemaDecl.cpp:157
@@ +156,3 @@
+/// case.
+static IdNameLookupResult lookupIdNameInDependentClass(
+ Sema &S, const IdentifierInfo &II, SourceLocation NameLoc,
----------------
`IdName` doesn't really mean anything. Drop the `Id` here? Also, this name suggests that we're doing a normal lookup into a dependent class, following standard rules, and producing a list of declarations, which isn't correct.
Something like `classifyNameInKnownDependentBases` might be a better name.
================
Comment at: lib/Sema/SemaDecl.cpp:158
@@ +157,3 @@
+static IdNameLookupResult lookupIdNameInDependentClass(
+ Sema &S, const IdentifierInfo &II, SourceLocation NameLoc,
+ const CXXRecordDecl *RD, bool SupposeExistInBaseParm,
----------------
This interface would make more sense if it took a `DeclarationName` rather than a `const IdentifierInfo &`.
================
Comment at: lib/Sema/SemaDecl.cpp:159
@@ +158,3 @@
+ Sema &S, const IdentifierInfo &II, SourceLocation NameLoc,
+ const CXXRecordDecl *RD, bool SupposeExistInBaseParm,
+ const llvm::function_ref<bool(NamedDecl *)> &CheckKind) {
----------------
Maybe something like `AssumeExistsInUnknownDependentBase` would be clearer than `SupposeExistInBaseParm`? But this flag seems to be unnecessary -- instead, the callers can distinguish between `NotFound` and `NotFoundWithUnknownBase`, or not, as necessary.
================
Comment at: lib/Sema/SemaDecl.cpp:166
@@ +165,3 @@
+ for (NamedDecl *ND : RD->lookup(&II)) {
+ if (!CheckKind(ND))
+ return IdNameLookupResult::FoundNotSpecifiedKind;
----------------
rsmith wrote:
> If we find a type and a non-type, the type should win, per [basic.scope.declarative]p4 bullet 2.
You should probably use `ND->getUnderlyingDecl()` here to handle //using-declaration//s.
================
Comment at: lib/Sema/SemaDecl.cpp:166-167
@@ +165,4 @@
+ for (NamedDecl *ND : RD->lookup(&II)) {
+ if (!CheckKind(ND))
+ return IdNameLookupResult::FoundNotSpecifiedKind;
+ FoundDecl = IdNameLookupResult::FoundSpecifiedKind;
----------------
If we find a type and a non-type, the type should win, per [basic.scope.declarative]p4 bullet 2.
================
Comment at: lib/Sema/SemaDecl.cpp:175
@@ -153,2 +174,3 @@
const CXXRecordDecl *BaseRD = nullptr;
- if (auto *BaseTT = Base.getType()->getAs<TagType>())
+ auto BaseTy = S.getASTContext().getCanonicalType(Base.getType());
+ if (const TagType *BaseTT = BaseTy->getAs<TagType>())
----------------
The `getCanonicalType` call here appears to be redundant.
================
Comment at: lib/Sema/SemaDecl.cpp:180
@@ -157,2 +179,3 @@
+ BaseTy->getAs<TemplateSpecializationType>()) {
// Look for type decls in dependent base classes that have known primary
// templates.
----------------
Comment seems out of date.
================
Comment at: lib/Sema/SemaDecl.cpp:192-193
@@ -168,1 +191,4 @@
BaseRD = BasePrimaryTemplate;
+ } else if (SupposeExistInBaseParm &&
+ BaseTy->getAs<TemplateTypeParmType>() &&
+ FoundDecl == IdNameLookupResult::NotFound) {
----------------
This seems like a strange case to treat specially. Why would you handle a base class of type `T` here but not a base class of type `T::type`? (Why don't we do this for all dependent base classes that we can't heuristically perform lookup into?)
================
Comment at: lib/Sema/SemaDecl.cpp:219
@@ +218,3 @@
+static bool
+isDeclInDependentClass(Sema &S, CXXScopeSpec &SS, SourceLocation Loc,
+ const IdentifierInfo &II, bool SupposeExistInBaseParm,
----------------
Again, "dependent class" isn't the right term here, because normal lookup in a class template definition looks for names in a dependent class.
================
Comment at: lib/Sema/SemaDecl.cpp:219
@@ +218,3 @@
+static bool
+isDeclInDependentClass(Sema &S, CXXScopeSpec &SS, SourceLocation Loc,
+ const IdentifierInfo &II, bool SupposeExistInBaseParm,
----------------
rsmith wrote:
> Again, "dependent class" isn't the right term here, because normal lookup in a class template definition looks for names in a dependent class.
Please keep the parameter order consistent between this class and the previous one. Putting the class name / scope specifier before the name being looked up seems like the better order.
================
Comment at: lib/Sema/SemaDecl.cpp:226-227
@@ +225,4 @@
+ auto *DC = S.computeDeclContext(SS, /*EnteringContext=*/true);
+ if (!DC)
+ DC = S.CurContext;
+ for (; DC && FoundDecl == IdNameLookupResult::NotFound;
----------------
This doesn't look right. If I have
template<typename T> struct X { typedef T type; };
then lookup of `type` within `X<T*>::` should presumably look in the primary template for `X` rather than looking in the current context.
================
Comment at: lib/Sema/SemaDecl.cpp:229
@@ -202,2 +228,3 @@
+ for (; DC && FoundDecl == IdNameLookupResult::NotFound;
DC = DC->getParent()) {
// Look for type decls in dependent base classes that have known primary
----------------
This should be `DC->getLookupParent()`.
================
Comment at: lib/Sema/SemaDecl.cpp:233
@@ -205,2 +232,3 @@
RD = dyn_cast<CXXRecordDecl>(DC);
if (RD && RD->getDescribedClassTemplate())
+ FoundDecl = lookupIdNameInDependentClass(
----------------
This requires `RD` to be the pattern of a class template; I would think that class template partial specializations, member classes of class templates, and local classes within function templates should get the same handling. You can just check `RD->isDependentContext()` to catch all of those cases.
================
Comment at: lib/Sema/SemaDecl.cpp:239-245
@@ +238,9 @@
+ FoundDecl == IdNameLookupResult::NotFoundWithUnknownBase) {
+ if (SS.isEmpty()) {
+ auto *NNS =
+ NestedNameSpecifier::Create(S.getASTContext(),
+ /*Prefix=*/nullptr,
+ /*Template=*/false, RD->getTypeForDecl());
+ SS.MakeTrivial(S.getASTContext(), NNS, SourceRange(Loc, Loc));
+ }
+ return true;
----------------
It's very strange for a function that looks like a simple predicate to modify its arguments in-place like this. How about making this function return a `NestedNameSpecifier *` or `CXXRecordDecl *` instead?
================
Comment at: lib/Sema/SemaDecl.cpp:252
@@ +251,3 @@
+
+bool Sema::isClassTemplateInClassWithAnyDependentBase(
+ CXXScopeSpec &SS, SourceLocation Loc, const IdentifierInfo &II) {
----------------
Can we somehow suggest in this name that the answer is uncertain if we don't know what all the dependent bases look like? `isClassTemplateInKnownDependentBase` perhaps?
================
Comment at: lib/Sema/SemaDecl.cpp:256
@@ +255,3 @@
+ *this, SS, Loc, II, /*SupposeExistInBaseParm=*/false,
+ [](NamedDecl *ND) -> bool { return isa<ClassTemplateDecl>(ND); });
+}
----------------
Should we allow other type templates, such as `AliasTemplateDecl` here too?
================
Comment at: lib/Sema/SemaDecl.cpp:288-290
@@ -217,7 +287,5 @@
ASTContext &Context = S.Context;
- auto *NNS = NestedNameSpecifier::Create(Context, nullptr, false,
- cast<Type>(Context.getRecordType(RD)));
- QualType T = Context.getDependentNameType(ETK_Typename, NNS, &II);
-
- CXXScopeSpec SS;
- SS.MakeTrivial(Context, NNS, SourceRange(NameLoc));
+ if (IsUnqualifiedLookup)
+ S.Diag(NameLoc, diag::ext_found_via_dependent_base_lookup)
+ << &II << S.computeDeclContext(SS, /*EnteringContext=*/true);
+ QualType T =
----------------
Why don't we emit a diagnostic here for qualified lookup?
================
Comment at: lib/Sema/SemaDecl.cpp:394-395
@@ +393,4 @@
+
+ // For unqualified lookup in a class template in MSVC mode, look into
+ // dependent base classes where the primary class template is known.
+ CXXScopeSpec NewSS;
----------------
Comment seems out of date.
================
Comment at: lib/Sema/SemaDecl.cpp:827-828
@@ -745,3 +826,4 @@
// For unqualified lookup in a class template in MSVC mode, look into
// dependent base classes where the primary class template is known.
+ CXXScopeSpec NewSS = SS;
----------------
Comment seems out of date.
================
Comment at: lib/Sema/SemaExpr.cpp:1959
@@ +1958,3 @@
+ if (!ObjectType.isNull()) {
+ // If ObjectType is defined - get cxx scope from this type.
+ RD = dyn_cast_or_null<CXXRecordDecl>(computeDeclContext(ObjectType));
----------------
Please remove 'cxx' from the comments here.
================
Comment at: lib/Sema/SemaExpr.cpp:1988-1993
@@ +1987,8 @@
+ IsLastChanceToRecover)) {
+ // Found specified member decl in dependent class.
+ if (ObjectType.isNull() && SS && SS->isEmpty()) {
+ *SS = TempSS;
+ if (II)
+ Diag(Loc, diag::ext_found_via_dependent_base_lookup)
+ << II << computeDeclContext(*SS, /*EnteringContext=*/true);
+ }
----------------
A function named `is...` should not be mutating state and producing diagnostics like this. Can you move the actual recovery code into the caller, or rename this function to reflect what it does (`recoverFromUnknownMemberLookup` or something)?
================
Comment at: lib/Sema/SemaExpr.cpp:2105
@@ -2104,5 +2104,3 @@
if (R.empty() && !ADL) {
- if (SS.isEmpty() && getLangOpts().MSVCCompat) {
- if (Expr *E = recoverFromMSUnqualifiedLookup(*this, Context, NameInfo,
- TemplateKWLoc, TemplateArgs))
- return E;
+ // In Microsoft mode, if we are inside a template class whose parent class
+ // has dependent base classes, and we can't resolve an unqualified
----------------
"class template", not "template class", "base class" not "parent class". Also, don't you mean "if we're inside a class template that has dependent base classes"?
================
Comment at: lib/Sema/SemaExpr.cpp:2112-2122
@@ -2109,1 +2111,13 @@
+ if (SS.isEmpty() && getLangOpts().MSVCCompat &&
+ isInClassWithAnyDependentBase(NameLoc, /*IsLastChanceToRecover=*/true,
+ &SS, QualType(), II)) {
+ if (getCurrentThisType().isNull())
+ return DependentScopeDeclRefExpr::Create(
+ Context, SS.getWithLocInContext(Context), TemplateKWLoc, NameInfo,
+ TemplateArgs);
+ return CXXDependentScopeMemberExpr::Create(
+ Context, /*This=*/nullptr, getCurrentThisType(),
+ /*IsArrow=*/true,
+ /*Op=*/SourceLocation(), NestedNameSpecifierLoc(), TemplateKWLoc,
+ /*FirstQualifierInScope=*/nullptr, NameInfo, TemplateArgs);
}
----------------
The checking performed here should depend on whether `this` is available. When walking the current class and its base classes, you're allowed to find members that need `this` and members that don't, but when you recurse to surrounding scopes, you should stop allowing members that need `this`.
================
Comment at: lib/Sema/SemaTemplate.cpp:184-186
@@ +183,5 @@
+ // typecasts/constructor calls:
+ // if (Templ<T>(a))
+ // ^~~~~
+ // typename
+ // use of undeclared identifier 'Templ'
----------------
This is a weird fixit: it's not syntactically vaild to put `typename` there. Do we really suggest that?
================
Comment at: test/SemaCXX/MicrosoftCompatibility.cpp:169-176
@@ -168,10 +168,10 @@
- A<T>::TYPE a1; // expected-warning {{missing 'typename' prior to dependent type name}}
- Base1::TYPE a2; // expected-warning {{missing 'typename' prior to dependent type name}}
+ A<T>::TYPE a1;
+ Base1::TYPE a2;
- B<U>::TYPE a3; // expected-warning {{missing 'typename' prior to dependent type name}}
- Base2::TYPE a4; // expected-warning {{missing 'typename' prior to dependent type name}}
+ B<U>::TYPE a3;
+ Base2::TYPE a4;
- A<U>::TYPE a5; // expected-error {{missing 'typename' prior to dependent type name}}
- Base3::TYPE a6; // expected-error {{missing 'typename' prior to dependent type name}}
};
----------------
Why do we no longer give these warnings?
================
Comment at: test/SemaTemplate/typename-specifier.cpp:214
@@ -213,2 +213,3 @@
-// expected-error at +1 {{missing 'typename' prior to dependent type name 'S<int>::type'}}
+#ifndef MSVC
+// expected-error at +2 {{missing 'typename' prior to dependent type name 'S<int>::type'}}
----------------
Shouldn't there be a warning here for the MSVC case?
http://reviews.llvm.org/D8100
EMAIL PREFERENCES
http://reviews.llvm.org/settings/panel/emailpreferences/
More information about the cfe-commits
mailing list