[PATCH] [MSVC] Better lookup into dependent bases for unspecified declaration (also fixes http://llvm.org/PR22066)

[Reid Kleckner]

I'm still not 100% sure what's going on here. In particular, I'd like to better understand the meaning of IdNameLookupResult.

Do you think you can split out a small change that adds the "insert typename here" suggestions? That would be a mostly mechanical change to update test results that significantly reduces the diff.


================
Comment at: include/clang/Sema/Sema.h:3518
@@ +3517,3 @@
+                                     CXXScopeSpec *SS = nullptr,
+                                     ParsedType ObjectType = ParsedType(),
+                                     const IdentifierInfo *II = nullptr);
----------------
Nobody in Parse calls this, so this can be QualType instead of ParsedType.

================
Comment at: include/clang/Sema/Sema.h:3519
@@ +3518,3 @@
+                                     ParsedType ObjectType = ParsedType(),
+                                     const IdentifierInfo *II = nullptr);
+
----------------
In this change, all call sites pass all of these parameters. Can you drop the default values?

================
Comment at: lib/Sema/SemaDecl.cpp:142-143
@@ -140,7 +141,4 @@
 /// dependent class.
-/// \return \a NotFound if no any decls is found, \a FoundNotType if found not a
-/// type decl, \a FoundType if only type decls are found.
-static UnqualifiedTypeNameLookupResult
-lookupUnqualifiedTypeNameInBase(Sema &S, const IdentifierInfo &II,
-                                SourceLocation NameLoc,
-                                const CXXRecordDecl *RD) {
+/// \return \a NotFound if no any decls is found, \a FoundNotSpecifiedKind if
+/// found not a type decl, \a FoundSpecifiedKind if only type decls are found.
+static IdNameLookupResult lookupIdNameInDependentClass(
----------------
This needs editing. I think you might be better off documenting each IdNameLookupResult individually.

================
Comment at: lib/Sema/SemaDecl.cpp:147
@@ -147,1 +146,3 @@
+    const CXXRecordDecl *RD, bool SupposeExistInBaseParm,
+    const std::function<bool(NamedDecl *)> &CheckKind) {
   if (!RD->hasDefinition())
----------------
I think we can use a simple function pointer without using too much power. Alternatively, llvm::function_ref is lighter weight.

================
Comment at: lib/Sema/SemaDecl.cpp:250-257
@@ +249,10 @@
+                                               bool SupposeExistInBaseParm) {
+  return isDeclInDependentClass(*this, SS, Loc, II, SupposeExistInBaseParm,
+                                [NonInstanceMemberOnly](NamedDecl *ND) -> bool {
+                                  if (ND->isCXXClassMember())
+                                    return NonInstanceMemberOnly
+                                               ? !ND->isCXXInstanceMember()
+                                               : true;
+                                  return false;
+                                });
+}
----------------
Looks like you can hoist the NonInstanceMemberOnly check out, and then remove the need for std::function. Consider:
  if (NonInstanceMemberOnly)
    return isDeclInDependentClass(
        *this, SS, Loc, II, SupposeExistInBaseParm, [](NamedDecl *ND) {
          return ND->isCXXClassMember() && !ND->isCXXInstanceMember();
        });
  return isDeclInDependentClass(
      *this, SS, Loc, II, SupposeExistInBaseParm,
      [](NamedDecl *ND) { return ND->isCXXClassMember(); });


================
Comment at: lib/Sema/SemaExpr.cpp:1959
@@ +1958,3 @@
+  CXXRecordDecl *RD = nullptr;
+  if (ObjectType && !ObjectType.get().isNull()) {
+    // If ObjectType is defined - get cxx scope from this type.
----------------
These conditions are equivalent, right? This can be `if (ObjectType)`

================
Comment at: lib/Sema/SemaExpr.cpp:1990
@@ +1989,3 @@
+    // Found specified member decl in dependent class.
+    if ((!ObjectType || ObjectType.get().isNull()) && SS && SS->isEmpty()) {
+      *SS = TempSS;
----------------
Ditto, `!ObjectType && SS && SS->isEmpty()`

================
Comment at: lib/Sema/SemaExpr.cpp:2106-2110
@@ -2104,5 +2105,7 @@
   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
+    /// identifier, then assume the identifier is a member of a dependent base
+    /// class. We can only recover successfully in static methods, instance
+    /// methods, and other contexts where 'this' is available.  This doesn't
+    /// precisely match MSVC's instantiation model, but it's close enough.
----------------
No need to use doxygen style /// comments in a function

http://reviews.llvm.org/D8100

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