HomePhabricator

clang/Modules: Refactor CompilerInstance::loadModule, NFC

Authored by dexonsmith on Nov 21 2019, 10:39 AM.

Description

clang/Modules: Refactor CompilerInstance::loadModule, NFC

Refactor the logic on CompilerInstance::loadModule and a couple of
surrounding methods in order to clarify what's going on.

  • Rename ModuleLoader::loadModuleFromSource to compileModuleFromSource and fix its documentation, since it never loads a module. It just creates/compiles one.
  • Rename one of the overloads of compileModuleImpl to compileModule, making it more obvious which one calls the other.
  • Rename compileAndLoadModule to compileModuleAndReadAST. This clarifies the relationship between this helper and its caller, CompilerInstance::loadModule (the old name implied the opposite relationship). It also (correctly) indicates that more needs to be done to load the module than this function is responsible for.
  • Split findOrCompileModuleAndReadAST out of loadModule. Besides reducing nesting for this code thanks to early returns and the like, this refactor clarifies the logic in loadModule, particularly around calls to ModuleMap::cacheModuleLoad and ModuleMap::getCachedModuleLoad. findOrCompileModuleAndReadAST also breaks early if the initial ReadAST call returns Missing or OutOfDate, allowing the last ditch call to compileModuleAndReadAST to come at the end of the function body.
    • Additionally split out selectModuleSource, clarifying the logic due to early returns.
    • Add ModuleLoadResult::isNormal and OtherUncachedFailure, so that loadModule knows whether to cache the result. OtherUncachedFailure was added to keep this patch NFC, but there's a chance that these cases were uncached by accident, through copy/paste/modify failures. These should be audited as a follow-up (maybe we can eliminate this case).
    • Do *not* lift the setting of ModuleLoadFailed = true to loadModule because there isn't a clear pattern for when it's set. This should be reconsidered in a follow-up, in case it would be correct to set ModuleLoadFailed whenever no module is returned and the result is either Normal or OtherUncachedFailure.
  • Add some header documentation where it was missing, and fix it where it was wrong.

This should have no functionality change.

https://reviews.llvm.org/D70556

Details

Committed
dexonsmithNov 22 2019, 6:23 PM
Parents
rG62335188f3ab: gn build: Reland c52efdc5, "gn build: (manually) merge b5913e6d2f"
Branches
Unknown
Tags
Unknown