[modules] Rearrange and unify the way we determine if we need to pull in

redeclaration chains when pulling in a declaration. We need the redecl chain
unless we know some other declaration will trigger it to be pulled in; that
happens if our originally-canonical declaration had all the knowledge that
we have (and isn't us).

llvm-svn: 213043

clang/lib/Serialization/ASTReaderDecl.cpp

@@ -110,8 +110,8 @@ namespace clang {
     /// chain and to introduce it into the list of pending redeclaration chains
     /// on destruction.
     ///
-    /// The caller can choose not to introduce this ID into the redeclaration
+    /// The caller can choose not to introduce this ID into the list of pending
-    /// chain by calling \c suppress().
+    /// redeclaration chains by calling \c suppress().
     class RedeclarableResult {
       ASTReader &Reader;
       GlobalDeclID FirstID;
@@ -1134,8 +1134,6 @@ void ASTDeclReader::VisitNamespaceDecl(NamespaceDecl *D) {
   D->setInline(Record[Idx++]);
   D->LocStart = ReadSourceLocation(Record, Idx);
   D->RBraceLoc = ReadSourceLocation(Record, Idx);
-  // FIXME: At the point of this call, D->getCanonicalDecl() returns 0.
-  mergeRedeclarable(D, Redecl);
 
   if (Redecl.getFirstID() == ThisDeclID) {
     // Each module has its own anonymous namespace, which is disjoint from
@@ -1149,6 +1147,8 @@ void ASTDeclReader::VisitNamespaceDecl(NamespaceDecl *D) {
     // been deserialized.
     D->AnonOrFirstNamespaceAndInline.setPointer(D->getFirstDecl());
   }
+
+  mergeRedeclarable(D, Redecl);
 }
 
 void ASTDeclReader::VisitNamespaceAliasDecl(NamespaceAliasDecl *D) {
@@ -2009,14 +2009,25 @@ ASTDeclReader::VisitRedeclarable(Redeclarable<T> *D) {
 /// \brief Attempts to merge the given declaration (D) with another declaration
 /// of the same entity.
 template<typename T>
-void ASTDeclReader::mergeRedeclarable(Redeclarable<T> *D,
+void ASTDeclReader::mergeRedeclarable(Redeclarable<T> *DBase,
                                       RedeclarableResult &Redecl,
                                       DeclID TemplatePatternID) {
+  T *D = static_cast<T*>(DBase);
+  T *DCanon = D->getCanonicalDecl();
+  if (D != DCanon &&
+      (!Reader.getContext().getLangOpts().Modules ||
+       Reader.getOwningModuleFile(DCanon) == Reader.getOwningModuleFile(D))) {
+    // All redeclarations between this declaration and its originally-canonical
+    // declaration get pulled in when we load DCanon; we don't need to
+    // perform any more merging now.
+    Redecl.suppress();
+  }
+
   // If modules are not available, there is no reason to perform this merge.
   if (!Reader.getContext().getLangOpts().Modules)
     return;
 
-  if (FindExistingResult ExistingRes = findExisting(static_cast<T*>(D)))
+  if (FindExistingResult ExistingRes = findExisting(D))
     if (T *Existing = ExistingRes)
       mergeRedeclarable(D, Existing, Redecl, TemplatePatternID);
 }
@@ -2036,7 +2047,8 @@ void ASTDeclReader::mergeTemplatePattern(RedeclarableTemplateDecl *D,
                                          DeclID DsID) {
   auto *DPattern = D->getTemplatedDecl();
   auto *ExistingPattern = Existing->getTemplatedDecl();
-  RedeclarableResult Result(Reader, DsID, DPattern->getKind());
+  RedeclarableResult Result(Reader, DPattern->getCanonicalDecl()->getGlobalID(),
+                            DPattern->getKind());
   if (auto *DClass = dyn_cast<CXXRecordDecl>(DPattern)) {
     // Merge with any existing definition.
     // FIXME: This is duplicated in several places. Refactor.
@@ -2076,8 +2088,10 @@ void ASTDeclReader::mergeRedeclarable(Redeclarable<T> *DBase, T *Existing,
   T *ExistingCanon = Existing->getCanonicalDecl();
   T *DCanon = D->getCanonicalDecl();
   if (ExistingCanon != DCanon) {
+    assert(DCanon->getGlobalID() == Redecl.getFirstID());
+
     // Have our redeclaration link point back at the canonical declaration
-    // of the existing declaration, so that this declaration has the 
+    // of the existing declaration, so that this declaration has the
     // appropriate canonical declaration.
     D->RedeclLink = Redeclarable<T>::PreviousDeclLink(ExistingCanon);
 
@@ -2092,18 +2106,6 @@ void ASTDeclReader::mergeRedeclarable(Redeclarable<T> *DBase, T *Existing,
           DTemplate, assert_cast<RedeclarableTemplateDecl*>(ExistingCanon),
           TemplatePatternID);
 
-    // Don't introduce DCanon into the set of pending declaration chains.
-    Redecl.suppress();
-
-    // Introduce ExistingCanon into the set of pending declaration chains,
-    // if in fact it came from a module file.
-    if (ExistingCanon->isFromASTFile()) {
-      GlobalDeclID ExistingCanonID = ExistingCanon->getGlobalID();
-      assert(ExistingCanonID && "Unrecorded canonical declaration ID?");
-      if (Reader.PendingDeclChainsKnown.insert(ExistingCanonID))
-        Reader.PendingDeclChains.push_back(ExistingCanonID);
-    }
-
     // If this declaration was the canonical declaration, make a note of
     // that. We accept the linear algorithm here because the number of
     // unique canonical declarations of an entity should always be tiny.
@@ -2112,14 +2114,6 @@ void ASTDeclReader::mergeRedeclarable(Redeclarable<T> *DBase, T *Existing,
       if (std::find(Merged.begin(), Merged.end(), Redecl.getFirstID())
             == Merged.end())
         Merged.push_back(Redecl.getFirstID());
-
-      // If ExistingCanon did not come from a module file, introduce the
-      // first declaration that *does* come from a module file to the
-      // set of pending declaration chains, so that we merge this
-      // declaration.
-      if (!ExistingCanon->isFromASTFile() &&
-          Reader.PendingDeclChainsKnown.insert(Redecl.getFirstID()))
-        Reader.PendingDeclChains.push_back(Merged[0]);
     }
   }
 }
@@ -2422,6 +2416,8 @@ ASTDeclReader::FindExistingResult ASTDeclReader::findExisting(NamedDecl *D) {
   if (!Name) {
     // Don't bother trying to find unnamed declarations.
     FindExistingResult Result(Reader, D, /*Existing=*/nullptr);
+    // FIXME: We may still need to pull in the redeclaration chain; there can
+    // be redeclarations via 'decltype'.
     Result.suppress();
     return Result;
   }
@@ -3001,6 +2997,8 @@ namespace {
       // Visit each of the declarations.
       for (unsigned I = 0, N = SearchDecls.size(); I != N; ++I)
         searchForID(M, SearchDecls[I]);
+      // FIXME: If none of the SearchDecls had local IDs in this module, can
+      // we avoid searching any ancestor module files?
       return false;
     }
     
@@ -3308,6 +3306,10 @@ void ASTDeclReader::UpdateDecl(Decl *D, ModuleFile &ModuleFile,
       // FIXME: This doesn't send the right notifications if there are
       // ASTMutationListeners other than an ASTWriter.
 
+      // FIXME: We can't both pull in declarations (and thus create new pending
+      // redeclaration chains) *and* walk redeclaration chains in this function.
+      // We should defer the updates that require walking redecl chains.
+
       // Maintain AST consistency: any later redeclarations are used too.
       for (auto *Redecl = D->getMostRecentDecl(); /**/;
            Redecl = Redecl->getPreviousDecl()) {