r293398 - Modules: Enforce that ModuleManager::removeModules deletes the tail

[Duncan P. N. Exon Smith via cfe-commits]

Author: dexonsmith
Date: Sat Jan 28 17:02:12 2017
New Revision: 293398

URL: http://llvm.org/viewvc/llvm-project?rev=293398&view=rev
Log:
Modules: Enforce that ModuleManager::removeModules deletes the tail

ModuleManager::removeModules always deletes a tail of the
ModuleManager::Chain.  Change the API to enforce that so that we can
simplify the code inside.

There's no real functionality change, although there's a slight
performance hack to loop to the First deleted module instead of the
final module in the chain (skipping the about-to-be-deleted tail).

Also document something suspicious: we fail to clean deleted modules out
of ModuleFile::Imports.

Modified:
    cfe/trunk/include/clang/Serialization/ModuleManager.h
    cfe/trunk/lib/Serialization/ASTReader.cpp
    cfe/trunk/lib/Serialization/ModuleManager.cpp

Modified: cfe/trunk/include/clang/Serialization/ModuleManager.h
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Serialization/ModuleManager.h?rev=293398&r1=293397&r2=293398&view=diff
==============================================================================
--- cfe/trunk/include/clang/Serialization/ModuleManager.h (original)
+++ cfe/trunk/include/clang/Serialization/ModuleManager.h Sat Jan 28 17:02:12 2017
@@ -228,8 +228,8 @@ public:
                             ModuleFile *&Module,
                             std::string &ErrorStr);
 
-  /// \brief Remove the given set of modules.
-  void removeModules(ModuleIterator first, ModuleIterator last,
+  /// \brief Remove the modules starting from First (to the end).
+  void removeModules(ModuleIterator First,
                      llvm::SmallPtrSetImpl<ModuleFile *> &LoadedSuccessfully,
                      ModuleMap *modMap);
 

Modified: cfe/trunk/lib/Serialization/ASTReader.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Serialization/ASTReader.cpp?rev=293398&r1=293397&r2=293398&view=diff
==============================================================================
--- cfe/trunk/lib/Serialization/ASTReader.cpp (original)
+++ cfe/trunk/lib/Serialization/ASTReader.cpp Sat Jan 28 17:02:12 2017
@@ -3643,11 +3643,10 @@ ASTReader::ASTReadResult ASTReader::Read
     for (const ImportedModule &IM : Loaded)
       LoadedSet.insert(IM.Mod);
 
-    ModuleMgr.removeModules(ModuleMgr.begin() + NumModules, ModuleMgr.end(),
-                            LoadedSet,
+    ModuleMgr.removeModules(ModuleMgr.begin() + NumModules, LoadedSet,
                             Context.getLangOpts().Modules
-                              ? &PP.getHeaderSearchInfo().getModuleMap()
-                              : nullptr);
+                                ? &PP.getHeaderSearchInfo().getModuleMap()
+                                : nullptr);
 
     // If we find that any modules are unusable, the global index is going
     // to be out-of-date. Just remove it.

Modified: cfe/trunk/lib/Serialization/ModuleManager.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Serialization/ModuleManager.cpp?rev=293398&r1=293397&r2=293398&view=diff
==============================================================================
--- cfe/trunk/lib/Serialization/ModuleManager.cpp (original)
+++ cfe/trunk/lib/Serialization/ModuleManager.cpp Sat Jan 28 17:02:12 2017
@@ -184,32 +184,35 @@ ModuleManager::addModule(StringRef FileN
 }
 
 void ModuleManager::removeModules(
-    ModuleIterator first, ModuleIterator last,
+    ModuleIterator First,
     llvm::SmallPtrSetImpl<ModuleFile *> &LoadedSuccessfully,
     ModuleMap *modMap) {
-  if (first == last)
+  auto Last = end();
+  if (First == Last)
     return;
 
+
   // Explicitly clear VisitOrder since we might not notice it is stale.
   VisitOrder.clear();
 
   // Collect the set of module file pointers that we'll be removing.
   llvm::SmallPtrSet<ModuleFile *, 4> victimSet(
-      (llvm::pointer_iterator<ModuleIterator>(first)),
-      (llvm::pointer_iterator<ModuleIterator>(last)));
+      (llvm::pointer_iterator<ModuleIterator>(First)),
+      (llvm::pointer_iterator<ModuleIterator>(Last)));
 
   auto IsVictim = [&](ModuleFile *MF) {
     return victimSet.count(MF);
   };
   // Remove any references to the now-destroyed modules.
-  for (unsigned i = 0, n = Chain.size(); i != n; ++i) {
-    Chain[i]->ImportedBy.remove_if(IsVictim);
-  }
+  //
+  // FIXME: this should probably clean up Imports as well.
+  for (auto I = begin(); I != First; ++I)
+    I->ImportedBy.remove_if(IsVictim);
   Roots.erase(std::remove_if(Roots.begin(), Roots.end(), IsVictim),
               Roots.end());
 
   // Remove the modules from the PCH chain.
-  for (auto I = first; I != last; ++I) {
+  for (auto I = First; I != Last; ++I) {
     if (!I->isModule()) {
       PCHChain.erase(std::find(PCHChain.begin(), PCHChain.end(), &*I),
                      PCHChain.end());
@@ -218,7 +221,7 @@ void ModuleManager::removeModules(
   }
 
   // Delete the modules and erase them from the various structures.
-  for (ModuleIterator victim = first; victim != last; ++victim) {
+  for (ModuleIterator victim = First; victim != Last; ++victim) {
     Modules.erase(victim->File);
 
     if (modMap) {
@@ -236,8 +239,7 @@ void ModuleManager::removeModules(
   }
 
   // Delete the modules.
-  Chain.erase(Chain.begin() + (first - begin()),
-              Chain.begin() + (last - begin()));
+  Chain.erase(Chain.begin() + (First - begin()), Chain.end());
 }
 
 void