[Sema] Refactor LookupVisibleDecls. NFC

[ilya-biryukov]

Summary

We accumulated some configuration parameters for LookupVisibleDecls that
are being passed unchanged to recursive calls, e.g. LoadExternal and
IncludeDependentBases.

At the same time, there is a bunch of parameters that can change in the
recursive invocations.

It is hard to tell the difference between those groups, making the code
hard to follow.

This change introduces a helper struct and factors out the non-changing
bits into fields, making recursive calls in the implementation code easier
to read.

[ilya-biryukov]

• Remove accidental change from revision

[ilya-biryukov]

This is just a proposal, there are probably other ways to reach better readability, e.g. group some of those parameters into a struct.
But let me know what you think, happy to refactor in a slightly different manner or simply drop this revision.

[sammccall]

This looks reasonable to me, there are a couple of variations you might think about:

• also treat QualifiedNameLookup as an option, and override in places with an RAII pattern like ScopedOverride Unqual(QualifiedNameLookup, false) (why don't we have a class like that?). This would further reduce args.
• put the options in a LookupOpts struct and pass it by const reference - this is a less invasive change

[ilya-biryukov]

In D65752#1623914, @sammccall wrote:

This looks reasonable to me, there are a couple of variations you might think about:

• also treat QualifiedNameLookup as an option, and override in places with an RAII pattern like ScopedOverride Unqual(QualifiedNameLookup, false) (why don't we have a class like that?). This would further reduce args.

I actually think it's good to have it as an explicit option. Qualified and unqualified lookup are so vastly different use-cases, that having the flag that discriminates them as a function parameter looks like a better trade-off.
Besides, RAII objects are more code and more complicated.

• put the options in a LookupOpts struct and pass it by const reference - this is a less invasive change

Also an option. I personally like the current version more even though it's more invasive; it adds more structure IMO.

Those decisions are personal preferences, I don't have strong opinions there.
@doug.gregor as an original author of the code, do you have any opinions on where this code should go?

[sammccall]

Agreed, this seems fine as is.

cfe/trunk/lib/Sema/SemaLookup.cpp

for (auto *D : Pos->second) {
       // We've found a declaration that hides this one.
       return D;
     }
   }
 
   return nullptr;
 }
 
-static void LookupVisibleDecls(DeclContext *Ctx, LookupResult &Result,
-                               bool QualifiedNameLookup,
-                               bool InBaseClass,
-                               VisibleDeclConsumer &Consumer,
-                               VisibleDeclsRecord &Visited,
-                               bool IncludeDependentBases,
-                               bool LoadExternal) {
+class LookupVisibleHelper {
+public:
+  LookupVisibleHelper(VisibleDeclConsumer &Consumer, bool IncludeDependentBases,
+                      bool LoadExternal)
+      : Consumer(Consumer), IncludeDependentBases(IncludeDependentBases),
+        LoadExternal(LoadExternal) {}
+
+  void lookupVisibleDecls(Sema &SemaRef, Scope *S, Sema::LookupNameKind Kind,
+                          bool IncludeGlobalScope) {
+    // Determine the set of using directives available during
+    // unqualified name lookup.
+    Scope *Initial = S;
+    UnqualUsingDirectiveSet UDirs(SemaRef);
+    if (SemaRef.getLangOpts().CPlusPlus) {
+      // Find the first namespace or translation-unit scope.
+      while (S && !isNamespaceOrTranslationUnitScope(S))
+        S = S->getParent();
+
+      UDirs.visitScopeChain(Initial, S);
+    }
+    UDirs.done();
+
+    // Look for visible declarations.
+    LookupResult Result(SemaRef, DeclarationName(), SourceLocation(), Kind);
+    Result.setAllowHidden(Consumer.includeHiddenDecls());
+    if (!IncludeGlobalScope)
+      Visited.visitedContext(SemaRef.getASTContext().getTranslationUnitDecl());
+    ShadowContextRAII Shadow(Visited);
+    lookupInScope(Initial, Result, UDirs);
+  }
+
+  void lookupVisibleDecls(Sema &SemaRef, DeclContext *Ctx,
+                          Sema::LookupNameKind Kind, bool IncludeGlobalScope) {
+    LookupResult Result(SemaRef, DeclarationName(), SourceLocation(), Kind);
+    Result.setAllowHidden(Consumer.includeHiddenDecls());
+    if (!IncludeGlobalScope)
+      Visited.visitedContext(SemaRef.getASTContext().getTranslationUnitDecl());
+
+    ShadowContextRAII Shadow(Visited);
+    lookupInDeclContext(Ctx, Result, /*QualifiedNameLookup=*/true,
+                        /*InBaseClass=*/false);
+  }
+
+private:
+  void lookupInDeclContext(DeclContext *Ctx, LookupResult &Result,
+                           bool QualifiedNameLookup, bool InBaseClass) {
     if (!Ctx)
       return;
 
     // Make sure we don't visit the same context twice.
     if (Visited.visitedContext(Ctx->getPrimaryContext()))
       return;
 
     Consumer.EnteredContext(Ctx);
 
     // Outside C++, lookup results for the TU live on identifiers.
     if (isa<TranslationUnitDecl>(Ctx) &&
         !Result.getSema().getLangOpts().CPlusPlus) {
       auto &S = Result.getSema();
       auto &Idents = S.Context.Idents;
 
       // Ensure all external identifiers are in the identifier table.
       if (LoadExternal)
-      if (IdentifierInfoLookup *External = Idents.getExternalIdentifierLookup()) {
+        if (IdentifierInfoLookup *External =
+                Idents.getExternalIdentifierLookup()) {
           std::unique_ptr<IdentifierIterator> Iter(External->getIdentifiers());
-        for (StringRef Name = Iter->Next(); !Name.empty(); Name = Iter->Next())
+          for (StringRef Name = Iter->Next(); !Name.empty();
+               Name = Iter->Next())
             Idents.get(Name);
         }
 
       // Walk all lookup results in the TU for each identifier.
       for (const auto &Ident : Idents) {
         for (auto I = S.IdResolver.begin(Ident.getValue()),
                   E = S.IdResolver.end();
              I != E; ++I) {
           if (S.IdResolver.isDeclInScope(*I, Ctx)) {
             if (NamedDecl *ND = Result.getAcceptableDecl(*I)) {
               Consumer.FoundDecl(ND, Visited.checkHidden(ND), Ctx, InBaseClass);
               Visited.add(ND);
             }
           }
         }
       }
 
       return;
     }
 
     if (CXXRecordDecl *Class = dyn_cast<CXXRecordDecl>(Ctx))
       Result.getSema().ForceDeclarationOfImplicitMembers(Class);
 
     // We sometimes skip loading namespace-level results (they tend to be huge).
     bool Load = LoadExternal ||
                 !(isa<TranslationUnitDecl>(Ctx) || isa<NamespaceDecl>(Ctx));
     // Enumerate all of the results in this context.
     for (DeclContextLookupResult R :
          Load ? Ctx->lookups()
               : Ctx->noload_lookups(/*PreserveInternalState=*/false)) {
       for (auto *D : R) {
         if (auto *ND = Result.getAcceptableDecl(D)) {
           Consumer.FoundDecl(ND, Visited.checkHidden(ND), Ctx, InBaseClass);
           Visited.add(ND);
         }
       }
     }
 
     // Traverse using directives for qualified name lookup.
     if (QualifiedNameLookup) {
       ShadowContextRAII Shadow(Visited);
       for (auto I : Ctx->using_directives()) {
         if (!Result.getSema().isVisible(I))
           continue;
-      LookupVisibleDecls(I->getNominatedNamespace(), Result,
-                         QualifiedNameLookup, InBaseClass, Consumer, Visited,
-                         IncludeDependentBases, LoadExternal);
+        lookupInDeclContext(I->getNominatedNamespace(), Result,
+                            QualifiedNameLookup, InBaseClass);
       }
     }
 
     // Traverse the contexts of inherited C++ classes.
     if (CXXRecordDecl *Record = dyn_cast<CXXRecordDecl>(Ctx)) {
       if (!Record->hasDefinition())
         return;
 
       for (const auto &B : Record->bases()) {
         QualType BaseType = B.getType();
 
         RecordDecl *RD;
         if (BaseType->isDependentType()) {
           if (!IncludeDependentBases) {
             // Don't look into dependent bases, because name lookup can't look
             // there anyway.
             continue;
           }
           const auto *TST = BaseType->getAs<TemplateSpecializationType>();
           if (!TST)
             continue;
           TemplateName TN = TST->getTemplateName();
           const auto *TD =
               dyn_cast_or_null<ClassTemplateDecl>(TN.getAsTemplateDecl());
           if (!TD)
             continue;
           RD = TD->getTemplatedDecl();
         } else {
           const auto *Record = BaseType->getAs<RecordType>();
           if (!Record)
             continue;
           RD = Record->getDecl();
         }
 
         // FIXME: It would be nice to be able to determine whether referencing
         // a particular member would be ambiguous. For example, given
         //
         //   struct A { int member; };
         //   struct B { int member; };
         //   struct C : A, B { };
         //
         //   void f(C *c) { c->### }
         //
         // accessing 'member' would result in an ambiguity. However, we
         // could be smart enough to qualify the member with the base
         // class, e.g.,
         //
         //   c->B::member
         //
         // or
         //
         //   c->A::member
 
         // Find results in this base class (and its bases).
         ShadowContextRAII Shadow(Visited);
-      LookupVisibleDecls(RD, Result, QualifiedNameLookup, /*InBaseClass=*/true,
-                         Consumer, Visited, IncludeDependentBases,
-                         LoadExternal);
+        lookupInDeclContext(RD, Result, QualifiedNameLookup,
+                            /*InBaseClass=*/true);
       }
     }
 
     // Traverse the contexts of Objective-C classes.
     if (ObjCInterfaceDecl *IFace = dyn_cast<ObjCInterfaceDecl>(Ctx)) {
       // Traverse categories.
       for (auto *Cat : IFace->visible_categories()) {
         ShadowContextRAII Shadow(Visited);
-      LookupVisibleDecls(Cat, Result, QualifiedNameLookup, false, Consumer,
-                         Visited, IncludeDependentBases, LoadExternal);
+        lookupInDeclContext(Cat, Result, QualifiedNameLookup,
+                            /*InBaseClass=*/false);
       }
 
       // Traverse protocols.
       for (auto *I : IFace->all_referenced_protocols()) {
         ShadowContextRAII Shadow(Visited);
-      LookupVisibleDecls(I, Result, QualifiedNameLookup, false, Consumer,
-                         Visited, IncludeDependentBases, LoadExternal);
+        lookupInDeclContext(I, Result, QualifiedNameLookup,
+                            /*InBaseClass=*/false);
       }
 
       // Traverse the superclass.
       if (IFace->getSuperClass()) {
         ShadowContextRAII Shadow(Visited);
-      LookupVisibleDecls(IFace->getSuperClass(), Result, QualifiedNameLookup,
-                         true, Consumer, Visited, IncludeDependentBases,
-                         LoadExternal);
+        lookupInDeclContext(IFace->getSuperClass(), Result, QualifiedNameLookup,
+                            /*InBaseClass=*/true);
       }
 
       // If there is an implementation, traverse it. We do this to find
       // synthesized ivars.
       if (IFace->getImplementation()) {
         ShadowContextRAII Shadow(Visited);
-      LookupVisibleDecls(IFace->getImplementation(), Result,
-                         QualifiedNameLookup, InBaseClass, Consumer, Visited,
-                         IncludeDependentBases, LoadExternal);
+        lookupInDeclContext(IFace->getImplementation(), Result,
+                            QualifiedNameLookup, InBaseClass);
       }
     } else if (ObjCProtocolDecl *Protocol = dyn_cast<ObjCProtocolDecl>(Ctx)) {
       for (auto *I : Protocol->protocols()) {
         ShadowContextRAII Shadow(Visited);
-      LookupVisibleDecls(I, Result, QualifiedNameLookup, false, Consumer,
-                         Visited, IncludeDependentBases, LoadExternal);
+        lookupInDeclContext(I, Result, QualifiedNameLookup,
+                            /*InBaseClass=*/false);
       }
     } else if (ObjCCategoryDecl *Category = dyn_cast<ObjCCategoryDecl>(Ctx)) {
       for (auto *I : Category->protocols()) {
         ShadowContextRAII Shadow(Visited);
-      LookupVisibleDecls(I, Result, QualifiedNameLookup, false, Consumer,
-                         Visited, IncludeDependentBases, LoadExternal);
+        lookupInDeclContext(I, Result, QualifiedNameLookup,
+                            /*InBaseClass=*/false);
       }
 
       // If there is an implementation, traverse it.
       if (Category->getImplementation()) {
         ShadowContextRAII Shadow(Visited);
-      LookupVisibleDecls(Category->getImplementation(), Result,
-                         QualifiedNameLookup, true, Consumer, Visited,
-                         IncludeDependentBases, LoadExternal);
+        lookupInDeclContext(Category->getImplementation(), Result,
+                            QualifiedNameLookup, /*InBaseClass=*/true);
       }
     }
   }
 
-static void LookupVisibleDecls(Scope *S, LookupResult &Result,
-                               UnqualUsingDirectiveSet &UDirs,
-                               VisibleDeclConsumer &Consumer,
-                               VisibleDeclsRecord &Visited,
-                               bool LoadExternal) {
+  void lookupInScope(Scope *S, LookupResult &Result,
+                     UnqualUsingDirectiveSet &UDirs) {
+    // No clients run in this mode and it's not supported. Please add tests and
+    // remove the assertion if you start relying on it.
+    assert(!IncludeDependentBases && "Unsupported flag for lookupInScope");
+
     if (!S)
       return;
 
     if (!S->getEntity() ||
-      (!S->getParent() &&
-       !Visited.alreadyVisitedContext(S->getEntity())) ||
+        (!S->getParent() && !Visited.alreadyVisitedContext(S->getEntity())) ||
         (S->getEntity())->isFunctionOrMethod()) {
       FindLocalExternScope FindLocals(Result);
       // Walk through the declarations in this Scope. The consumer might add new
       // decls to the scope as part of deserialization, so make a copy first.
       SmallVector<Decl *, 8> ScopeDecls(S->decls().begin(), S->decls().end());
       for (Decl *D : ScopeDecls) {
         if (NamedDecl *ND = dyn_cast<NamedDecl>(D))
           if ((ND = Result.getAcceptableDecl(ND))) {
             Consumer.FoundDecl(ND, Visited.checkHidden(ND), nullptr, false);
             Visited.add(ND);
           }
       }
     }
 
     // FIXME: C++ [temp.local]p8
     DeclContext *Entity = nullptr;
     if (S->getEntity()) {
       // Look into this scope's declaration context, along with any of its
       // parent lookup contexts (e.g., enclosing classes), up to the point
       // where we hit the context stored in the next outer scope.
       Entity = S->getEntity();
       DeclContext *OuterCtx = findOuterContext(S).first; // FIXME
 
       for (DeclContext *Ctx = Entity; Ctx && !Ctx->Equals(OuterCtx);
            Ctx = Ctx->getLookupParent()) {
         if (ObjCMethodDecl *Method = dyn_cast<ObjCMethodDecl>(Ctx)) {
           if (Method->isInstanceMethod()) {
             // For instance methods, look for ivars in the method's interface.
             LookupResult IvarResult(Result.getSema(), Result.getLookupName(),
-                                  Result.getNameLoc(), Sema::LookupMemberName);
+                                    Result.getNameLoc(),
+                                    Sema::LookupMemberName);
             if (ObjCInterfaceDecl *IFace = Method->getClassInterface()) {
-            LookupVisibleDecls(IFace, IvarResult, /*QualifiedNameLookup=*/false,
-                               /*InBaseClass=*/false, Consumer, Visited,
-                               /*IncludeDependentBases=*/false, LoadExternal);
+              lookupInDeclContext(IFace, IvarResult,
+                                  /*QualifiedNameLookup=*/false,
+                                  /*InBaseClass=*/false);
             }
           }
 
           // We've already performed all of the name lookup that we need
           // to for Objective-C methods; the next context will be the
           // outer scope.
           break;
         }
 
         if (Ctx->isFunctionOrMethod())
           continue;
 
-      LookupVisibleDecls(Ctx, Result, /*QualifiedNameLookup=*/false,
-                         /*InBaseClass=*/false, Consumer, Visited,
-                         /*IncludeDependentBases=*/false, LoadExternal);
+        lookupInDeclContext(Ctx, Result, /*QualifiedNameLookup=*/false,
+                            /*InBaseClass=*/false);
       }
     } else if (!S->getParent()) {
       // Look into the translation unit scope. We walk through the translation
       // unit's declaration context, because the Scope itself won't have all of
       // the declarations if we loaded a precompiled header.
-    // FIXME: We would like the translation unit's Scope object to point to the
-    // translation unit, so we don't need this special "if" branch. However,
-    // doing so would force the normal C++ name-lookup code to look into the
-    // translation unit decl when the IdentifierInfo chains would suffice.
-    // Once we fix that problem (which is part of a more general "don't look
-    // in DeclContexts unless we have to" optimization), we can eliminate this.
+      // FIXME: We would like the translation unit's Scope object to point to
+      // the translation unit, so we don't need this special "if" branch.
+      // However, doing so would force the normal C++ name-lookup code to look
+      // into the translation unit decl when the IdentifierInfo chains would
+      // suffice. Once we fix that problem (which is part of a more general
+      // "don't look in DeclContexts unless we have to" optimization), we can
+      // eliminate this.
       Entity = Result.getSema().Context.getTranslationUnitDecl();
-    LookupVisibleDecls(Entity, Result, /*QualifiedNameLookup=*/false,
-                       /*InBaseClass=*/false, Consumer, Visited,
-                       /*IncludeDependentBases=*/false, LoadExternal);
+      lookupInDeclContext(Entity, Result, /*QualifiedNameLookup=*/false,
+                          /*InBaseClass=*/false);
     }
 
     if (Entity) {
       // Lookup visible declarations in any namespaces found by using
       // directives.
       for (const UnqualUsingEntry &UUE : UDirs.getNamespacesFor(Entity))
-      LookupVisibleDecls(const_cast<DeclContext *>(UUE.getNominatedNamespace()),
-                         Result, /*QualifiedNameLookup=*/false,
-                         /*InBaseClass=*/false, Consumer, Visited,
-                         /*IncludeDependentBases=*/false, LoadExternal);
+        lookupInDeclContext(
+            const_cast<DeclContext *>(UUE.getNominatedNamespace()), Result,
+            /*QualifiedNameLookup=*/false,
+            /*InBaseClass=*/false);
     }
 
     // Lookup names in the parent scope.
     ShadowContextRAII Shadow(Visited);
-  LookupVisibleDecls(S->getParent(), Result, UDirs, Consumer, Visited,
-                     LoadExternal);
+    lookupInScope(S->getParent(), Result, UDirs);
   }
 
+private:
+  VisibleDeclsRecord Visited;
+  VisibleDeclConsumer &Consumer;
+  bool IncludeDependentBases;
+  bool LoadExternal;
+};
+
 void Sema::LookupVisibleDecls(Scope *S, LookupNameKind Kind,
                               VisibleDeclConsumer &Consumer,
                               bool IncludeGlobalScope, bool LoadExternal) {
-  // Determine the set of using directives available during
-  // unqualified name lookup.
-  Scope *Initial = S;
-  UnqualUsingDirectiveSet UDirs(*this);
-  if (getLangOpts().CPlusPlus) {
-    // Find the first namespace or translation-unit scope.
-    while (S && !isNamespaceOrTranslationUnitScope(S))
-      S = S->getParent();
-
-    UDirs.visitScopeChain(Initial, S);
-  }
-  UDirs.done();
-
-  // Look for visible declarations.
-  LookupResult Result(*this, DeclarationName(), SourceLocation(), Kind);
-  Result.setAllowHidden(Consumer.includeHiddenDecls());
-  VisibleDeclsRecord Visited;
-  if (!IncludeGlobalScope)
-    Visited.visitedContext(Context.getTranslationUnitDecl());
-  ShadowContextRAII Shadow(Visited);
-  ::LookupVisibleDecls(Initial, Result, UDirs, Consumer, Visited, LoadExternal);
+  LookupVisibleHelper H(Consumer, /*IncludeDependentBases=*/false,
+                        LoadExternal);
+  H.lookupVisibleDecls(*this, S, Kind, IncludeGlobalScope);
 }
 
 void Sema::LookupVisibleDecls(DeclContext *Ctx, LookupNameKind Kind,
                               VisibleDeclConsumer &Consumer,
                               bool IncludeGlobalScope,
                               bool IncludeDependentBases, bool LoadExternal) {
-  LookupResult Result(*this, DeclarationName(), SourceLocation(), Kind);
-  Result.setAllowHidden(Consumer.includeHiddenDecls());
-  VisibleDeclsRecord Visited;
-  if (!IncludeGlobalScope)
-    Visited.visitedContext(Context.getTranslationUnitDecl());
-  ShadowContextRAII Shadow(Visited);
-  ::LookupVisibleDecls(Ctx, Result, /*QualifiedNameLookup=*/true,
-                       /*InBaseClass=*/false, Consumer, Visited,
-                       IncludeDependentBases, LoadExternal);
+  LookupVisibleHelper H(Consumer, IncludeDependentBases, LoadExternal);
+  H.lookupVisibleDecls(*this, Ctx, Kind, IncludeGlobalScope);
 }
 
 /// LookupOrCreateLabel - Do a name lookup of a label with the specified name.
 /// If GnuLabelLoc is a valid source location, then this is a definition
 /// of an __label__ label name, otherwise it is a normal label definition
 /// or use.
 LabelDecl *Sema::LookupOrCreateLabel(IdentifierInfo *II, SourceLocation Loc,
                                      SourceLocation GnuLabelLoc) {