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

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

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG7c57a9bd7d4c: Modules: Simplify how 
DisableGeneratingGlobalModuleIndex is set, likely NFC (authored by dexonsmith).

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D101672/new/

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);
   }


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 

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

[Jan Svoboda via Phabricator via cfe-commits]

jansvoboda11 accepted this revision.
jansvoboda11 added a comment.
This revision is now accepted and ready to land.

LGTM.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D101672/new/

https://reviews.llvm.org/D101672

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

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

dexonsmith added a comment.

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).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D101672/new/

https://reviews.llvm.org/D101672

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[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 
.

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);
   }


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)
 <<