Modules: Simplify how DisableGeneratingGlobalModuleIndex is set, likely NFC

[dexonsmith]

Summary

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.

The extra cases where this is set are all some version of a fatal error,
and the only client of the field, shouldBuildGlobalModuleIndex, 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.

[dexonsmith]

Just fixed a typo in the description, where the second paragraph should start:

The extra cases where this is set are all some version of a fatal error,
and the only client of the field, shouldBuildGlobalModuleIndex, seems
to be unreachable in that case.

(and now does).

[jansvoboda11]

LGTM.

clang/lib/Frontend/CompilerInstance.cpp

ModuleLoadResult CompilerInstance::findOrCompileModuleAndReadAST(
   // Select the source and filename for loading the named module.
   std::string ModuleFilename;
   ModuleSource Source =
       selectModuleSource(M, ModuleName, ModuleFilename, BuiltModules, HS);
   if (Source == MS_ModuleNotFound) {
     // 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()) {
     if (M && M->HasIncompatibleModuleFile) {
       // We tried and failed to load a module file for this module. Fall
       // back to textual inclusion for its headers.
       return ModuleLoadResult::ConfigMismatch;
     }
 
     getDiagnostics().Report(ModuleNameLoc, diag::err_module_build_disabled)
         << ModuleName;
-    DisableGeneratingGlobalModuleIndex = true;
     return nullptr;
   }
 
   // Create an ASTReader on demand.
   if (!getASTReader())
     createASTReader();
 
   // Time how long it takes to load the module.
case ASTReader::Success: {
     M = HS.lookupModule(ModuleName, true, !IsInclusionDirective);
 
     // Check whether M refers to the file in the prebuilt module path.
     if (M && M->getASTFile())
       if (auto ModuleFile = FileMgr->getFile(ModuleFilename))
         if (*ModuleFile == M->getASTFile())
           return M;
 
-    DisableGeneratingGlobalModuleIndex = true;
     getDiagnostics().Report(ModuleNameLoc, diag::err_module_prebuilt)
         << ModuleName;
     return ModuleLoadResult();
   }
 
   case ASTReader::OutOfDate:
   case ASTReader::Missing:
     // The most interesting case.
     break;
 
   case ASTReader::ConfigurationMismatch:
     if (Source == MS_PrebuiltModulePath)
       // FIXME: We shouldn't be setting HadFatalFailure below if we only
       // produce a warning here!
       getDiagnostics().Report(SourceLocation(),
                               diag::warn_module_config_mismatch)
           << ModuleFilename;
     // Fall through to error out.
     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();
   }
 
   // ReadAST returned Missing or OutOfDate.
   if (Source != MS_ModuleCache) {
     // 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();
   }
 
   // The module file is missing or out-of-date. Build it.
   assert(M && "missing module, but trying to compile for cache");
 
   // Check whether there is a cycle in the module graph.
   ModuleBuildStack ModPath = getSourceManager().getModuleBuildStack();
   ModuleBuildStack::iterator Pos = ModPath.begin(), PosEnd = ModPath.end();
   for (; Pos != PosEnd; ++Pos) {
     if (Pos->first == ModuleName)
       break;
   }
 
   if (Pos != PosEnd) {
     SmallString<256> CyclePath;
     for (; Pos != PosEnd; ++Pos) {
       CyclePath += Pos->first;
       CyclePath += " -> ";
     }
     CyclePath += ModuleName;
 
     getDiagnostics().Report(ModuleNameLoc, diag::err_module_cycle)
         << ModuleName << CyclePath;
-    // FIXME: Should this set DisableGeneratingGlobalModuleIndex = true?
     return nullptr;
   }
 
   // Check whether we have already attempted to build this module (but
   // failed).
   if (getPreprocessorOpts().FailedModules &&
       getPreprocessorOpts().FailedModules->hasAlreadyFailed(ModuleName)) {
     getDiagnostics().Report(ModuleNameLoc, diag::err_module_not_built)
         << ModuleName << SourceRange(ImportLoc, ModuleNameLoc);
-    DisableGeneratingGlobalModuleIndex = true;
     return nullptr;
   }
 
   // Try to compile and then read the AST.
   if (!compileModuleAndReadAST(*this, ImportLoc, ModuleNameLoc, M,
                                ModuleFilename)) {
     assert(getDiagnostics().hasErrorOccurred() &&
            "undiagnosed error in compileModuleAndReadAST");
     if (getPreprocessorOpts().FailedModules)
       getPreprocessorOpts().FailedModules->addFailed(ModuleName);
-    DisableGeneratingGlobalModuleIndex = true;
     return nullptr;
   }
 
   // Okay, we've rebuilt and now loaded the module.
   return M;
 }
 
 ModuleLoadResult
if (auto MaybeModule = MM.getCachedModuleLoad(*Path[0].first)) {
     //    << ModuleName;
     //  DisableGeneratingGlobalModuleIndex = true;
     //  return ModuleLoadResult();
     //}
     MM.cacheModuleLoad(*Path[0].first, Module);
   } 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);
   }
 
   // If we never found the module, fail.  Otherwise, verify the module and link
   // it up.
   if (!Module)
     return ModuleLoadResult();