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

[Alexey Bataev]

Reid, I will remove all 'typename' related code.


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

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

================
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(
----------------
rnk wrote:
> This needs editing. I think you might be better off documenting each IdNameLookupResult individually.
Added an additional comment about SupposeFoundSpecifiedKind element and added comments to IdNameLookupResult enum and its elements.

================
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())
----------------
rnk wrote:
> I think we can use a simple function pointer without using too much power. Alternatively, llvm::function_ref is lighter weight.
Changed it to function_ref

================
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;
+                                });
+}
----------------
rnk wrote:
> 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(); });
> 
Ok, consider it done

================
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.
----------------
rnk wrote:
> These conditions are equivalent, right? This can be `if (ObjectType)`
Fixed

================
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;
----------------
rnk wrote:
> Ditto, `!ObjectType && SS && SS->isEmpty()`
Fixed

================
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.
----------------
rnk wrote:
> No need to use doxygen style /// comments in a function
Ok, fixed

http://reviews.llvm.org/D8100

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