[PATCH] D101672: Modules: Simplify how DisableGeneratingGlobalModuleIndex is set, likely NFC

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

dexonsmith created this revision.
dexonsmith added reviewers: jansvoboda11, Bigcheese, rsmith.
Herald added a subscriber: arphaman.
dexonsmith requested review of this revision.
Herald added a project: clang.

DisableGeneratingGlobalModuleIndex was being set by
CompilerInstance::findOrCompileModuleAndReadAST most of (but not all of)
the times it returned `nullptr` as a "normal" failure. Pull that up to
the caller, CompilerInstance::loadModule, to simplify the code. This
resolves a number of FIXMEs added during the refactoring in
5cca622310c10fdf6f921b6cce26f91d9f14c762 <https://reviews.llvm.org/rG5cca622310c10fdf6f921b6cce26f91d9f14c762>.

The extra cases where this is set are all some version of a fatal error,
and the only client of the field, findOrCompileModuleAndReadAST, seems
to be unreachable in that case. Even if there is some corner case where
this has an effect, it seems like the right/consistent behaviour.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D101672

Files:
  clang/lib/Frontend/CompilerInstance.cpp


Index: clang/lib/Frontend/CompilerInstance.cpp
===================================================================
--- clang/lib/Frontend/CompilerInstance.cpp
+++ clang/lib/Frontend/CompilerInstance.cpp
@@ -1683,7 +1683,6 @@
     // We can't find a module, error out here.
     getDiagnostics().Report(ModuleNameLoc, diag::err_module_not_found)
         << ModuleName << SourceRange(ImportLoc, ModuleNameLoc);
-    DisableGeneratingGlobalModuleIndex = true;
     return nullptr;
   }
   if (ModuleFilename.empty()) {
@@ -1695,7 +1694,6 @@
 
     getDiagnostics().Report(ModuleNameLoc, diag::err_module_build_disabled)
         << ModuleName;
-    DisableGeneratingGlobalModuleIndex = true;
     return nullptr;
   }
 
@@ -1742,7 +1740,6 @@
         if (*ModuleFile == M->getASTFile())
           return M;
 
-    DisableGeneratingGlobalModuleIndex = true;
     getDiagnostics().Report(ModuleNameLoc, diag::err_module_prebuilt)
         << ModuleName;
     return ModuleLoadResult();
@@ -1764,14 +1761,12 @@
     LLVM_FALLTHROUGH;
   case ASTReader::VersionMismatch:
   case ASTReader::HadErrors:
-    // FIXME: Should this set DisableGeneratingGlobalModuleIndex = true?
     ModuleLoader::HadFatalFailure = true;
     // FIXME: The ASTReader will already have complained, but can we shoehorn
     // that diagnostic information into a more useful form?
     return ModuleLoadResult();
 
   case ASTReader::Failure:
-    // FIXME: Should this set DisableGeneratingGlobalModuleIndex = true?
     ModuleLoader::HadFatalFailure = true;
     return ModuleLoadResult();
   }
@@ -1781,7 +1776,6 @@
     // We don't know the desired configuration for this module and don't
     // necessarily even have a module map. Since ReadAST already produces
     // diagnostics for these two cases, we simply error out here.
-    DisableGeneratingGlobalModuleIndex = true;
     return ModuleLoadResult();
   }
 
@@ -1806,7 +1800,6 @@
 
     getDiagnostics().Report(ModuleNameLoc, diag::err_module_cycle)
         << ModuleName << CyclePath;
-    // FIXME: Should this set DisableGeneratingGlobalModuleIndex = true?
     return nullptr;
   }
 
@@ -1816,7 +1809,6 @@
       getPreprocessorOpts().FailedModules->hasAlreadyFailed(ModuleName)) {
     getDiagnostics().Report(ModuleNameLoc, diag::err_module_not_built)
         << ModuleName << SourceRange(ImportLoc, ModuleNameLoc);
-    DisableGeneratingGlobalModuleIndex = true;
     return nullptr;
   }
 
@@ -1827,7 +1819,6 @@
            "undiagnosed error in compileModuleAndReadAST");
     if (getPreprocessorOpts().FailedModules)
       getPreprocessorOpts().FailedModules->addFailed(ModuleName);
-    DisableGeneratingGlobalModuleIndex = true;
     return nullptr;
   }
 
@@ -1878,11 +1869,10 @@
   } else {
     ModuleLoadResult Result = findOrCompileModuleAndReadAST(
         ModuleName, ImportLoc, ModuleNameLoc, IsInclusionDirective);
-    // FIXME: Can we pull 'DisableGeneratingGlobalModuleIndex = true' out of
-    // the return sequences for findOrCompileModuleAndReadAST and do it here
-    // (as long as the result is not a config mismatch)?  See FIXMEs there.
     if (!Result.isNormal())
       return Result;
+    if (!Result)
+      DisableGeneratingGlobalModuleIndex = true;
     Module = Result;
     MM.cacheModuleLoad(*Path[0].first, Module);
   }


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D101672.342067.patch
Type: text/x-patch
Size: 3304 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20210430/7d149e6a/attachment.bin>