[clang] 7c2afd5 - Modules: Remove ModuleLoader::OtherUncachedFailure, NFC

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

Author: Duncan P. N. Exon Smith
Date: 2021-05-13T10:10:46-07:00
New Revision: 7c2afd5899df876eaf5ffb485194dc58e92daf89

URL: https://github.com/llvm/llvm-project/commit/7c2afd5899df876eaf5ffb485194dc58e92daf89
DIFF: https://github.com/llvm/llvm-project/commit/7c2afd5899df876eaf5ffb485194dc58e92daf89.diff

LOG: Modules: Remove ModuleLoader::OtherUncachedFailure, NFC

5cca622310c10fdf6f921b6cce26f91d9f14c762 refactored
CompilerInstance::loadModule, splitting out
findOrCompileModuleAndReadAST, but was careful to avoid making any
functional changes. It added ModuleLoader::OtherUncachedFailure to
facilitate this and left behind FIXMEs asking why certain failures
weren't cached.

After a closer look, I think we can just remove this and simplify the
code. This changes the behaviour of the following (simplified) code from
CompilerInstance::loadModule, causing a failure to be cached more often:

```
  if (auto MaybeModule = MM.getCachedModuleLoad(*Path[0].first))
    return *MaybeModule;
  if (ModuleName == getLangOpts().CurrentModule)
    return MM.cacheModuleLoad(PP.lookupModule(...));
  ModuleLoadResult Result = findOrCompileModuleAndReadAST(...);
  if (Result.isNormal()) // This will be 'true' more often.
    return MM.cacheModuleLoad(..., Module);
  return Result;
```

`MM` here is a ModuleMap owned by the Preprocessor. Here are the cases
where `findOrCompileModuleAndReadAST` starts returning a "normal" failed
result:
- Emitted `diag::err_module_not_found`, where there's no module map
  found.
- Emitted `diag::err_module_build_disabled`, where implicitly building
  modules is disabled.
- Emitted `diag::err_module_cycle`, which detects module cycles in the
  implicit modules build system.
- Emitted `diag::err_module_not_built`, which avoids building a module
  in this CompilerInstance if another one tried and failed already.
- `compileModuleAndReadAST()` was called and failed to build.

The four errors are all fatal, and last item also reports a fatal error,
so it this extra caching has no functionality change... but even if it
did, it seems fine to cache these failed results within a ModuleMap
instance (note that each CompilerInstance has its own Preprocessor and
ModuleMap).

Differential Revision: https://reviews.llvm.org/D101667

Added: 
    

Modified: 
    clang/include/clang/Lex/ModuleLoader.h
    clang/lib/Frontend/CompilerInstance.cpp

Removed: 
    


################################################################################
diff  --git a/clang/include/clang/Lex/ModuleLoader.h b/clang/include/clang/Lex/ModuleLoader.h
index c1f7f068c0f1d..bf044e0e5f506 100644
--- a/clang/include/clang/Lex/ModuleLoader.h
+++ b/clang/include/clang/Lex/ModuleLoader.h
@@ -45,9 +45,6 @@ class ModuleLoadResult {
 
     // The module exists but cannot be imported due to a configuration mismatch.
     ConfigMismatch,
-
-    // We failed to load the module, but we shouldn't cache the failure.
-    OtherUncachedFailure,
   };
   llvm::PointerIntPair<Module *, 2, LoadResultKind> Storage;
 

diff  --git a/clang/lib/Frontend/CompilerInstance.cpp b/clang/lib/Frontend/CompilerInstance.cpp
index bcf9f9694f885..695c7e335fb40 100644
--- a/clang/lib/Frontend/CompilerInstance.cpp
+++ b/clang/lib/Frontend/CompilerInstance.cpp
@@ -1684,8 +1684,7 @@ ModuleLoadResult CompilerInstance::findOrCompileModuleAndReadAST(
     getDiagnostics().Report(ModuleNameLoc, diag::err_module_not_found)
         << ModuleName << SourceRange(ImportLoc, ModuleNameLoc);
     ModuleBuildFailed = true;
-    // FIXME: Why is this not cached?
-    return ModuleLoadResult::OtherUncachedFailure;
+    return nullptr;
   }
   if (ModuleFilename.empty()) {
     if (M && M->HasIncompatibleModuleFile) {
@@ -1697,8 +1696,7 @@ ModuleLoadResult CompilerInstance::findOrCompileModuleAndReadAST(
     getDiagnostics().Report(ModuleNameLoc, diag::err_module_build_disabled)
         << ModuleName;
     ModuleBuildFailed = true;
-    // FIXME: Why is this not cached?
-    return ModuleLoadResult::OtherUncachedFailure;
+    return nullptr;
   }
 
   // Create an ASTReader on demand.
@@ -1809,8 +1807,7 @@ ModuleLoadResult CompilerInstance::findOrCompileModuleAndReadAST(
     getDiagnostics().Report(ModuleNameLoc, diag::err_module_cycle)
         << ModuleName << CyclePath;
     // FIXME: Should this set ModuleBuildFailed = true?
-    // FIXME: Why is this not cached?
-    return ModuleLoadResult::OtherUncachedFailure;
+    return nullptr;
   }
 
   // Check whether we have already attempted to build this module (but
@@ -1820,8 +1817,7 @@ ModuleLoadResult CompilerInstance::findOrCompileModuleAndReadAST(
     getDiagnostics().Report(ModuleNameLoc, diag::err_module_not_built)
         << ModuleName << SourceRange(ImportLoc, ModuleNameLoc);
     ModuleBuildFailed = true;
-    // FIXME: Why is this not cached?
-    return ModuleLoadResult::OtherUncachedFailure;
+    return nullptr;
   }
 
   // Try to compile and then read the AST.
@@ -1832,8 +1828,7 @@ ModuleLoadResult CompilerInstance::findOrCompileModuleAndReadAST(
     if (getPreprocessorOpts().FailedModules)
       getPreprocessorOpts().FailedModules->addFailed(ModuleName);
     ModuleBuildFailed = true;
-    // FIXME: Why is this not cached?
-    return ModuleLoadResult::OtherUncachedFailure;
+    return nullptr;
   }
 
   // Okay, we've rebuilt and now loaded the module.