Page MenuHomePhabricator

clang/Modules: Refactor CompilerInstance::loadModule, NFC

Authored by dexonsmith on Nov 21 2019, 12:12 PM.



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.

Diff Detail

Event Timeline

dexonsmith created this revision.Nov 21 2019, 12:12 PM
dexonsmith marked 2 inline comments as done.Nov 21 2019, 12:17 PM
dexonsmith added inline comments.

I put this in the header to reduce how many parameters to pass in by-reference (it felt unnecessarily verbose). But it's possible to make it a static in the source file if that's better.


Note that this FIXME predates this patch (it has just moved).

jkorous added inline comments.Nov 21 2019, 2:01 PM

Just a typo - the comma at the end.

dexonsmith marked 2 inline comments as done.Nov 21 2019, 2:34 PM
dexonsmith added inline comments.

That comma was intentional, actually! Then the next time a value gets added/removed it doesn't unnecessarily cause differences to other liens :).

Bigcheese accepted this revision.Nov 22 2019, 5:32 PM

lgtm. Nice cleanup.

This revision is now accepted and ready to land.Nov 22 2019, 5:32 PM
bruno accepted this revision.Nov 22 2019, 6:06 PM

Nice! LGTM

dexonsmith closed this revision.Nov 22 2019, 6:25 PM
dexonsmith marked an inline comment as done.