benlangmuir added inline comments.
================ Comment at: clang/include/clang/Tooling/DependencyScanning/DependencyScanningService.h:50 + bool OptimizeArgs = false, + bool EagerLoadModules = false); ---------------- Since you're changing the default to lazy loading, please mention that in the commit message. ================ Comment at: clang/include/clang/Tooling/DependencyScanning/DependencyScanningTool.h:104 DependencyScanningWorker Worker; + bool EagerLoadModules; }; ---------------- Nit: How about adding an accessor to the DependencyScanningWorker instead of duplicating this state here? I find it easier to understand the tool when it has no state of its own and just wraps a worker. ================ Comment at: clang/lib/Tooling/DependencyScanning/ModuleDepCollector.cpp:71 + CI.getFrontendOpts().ModuleMapFiles.push_back( + ModularDeps[M]->ClangModuleMapFile); + } ---------------- I think we need to include this new state in `getModuleContextHash`. Here's my suggestion: * In `makeInvocationForModuleBuildWithoutOutputs` you can add the module map paths if `EagerLoadModules` is false. Those are all inputs, so it should be fine to do immediately, which will also include them in the context hash for free. * In `getModuleContextHash` hash just the `EagerLoadModules` bit. It's a bit subtle whether this is actually needed or not since adding the module map paths will also change the hash, but I think it's safer to include it since in the future we could e.g. drop duplicate module map paths, and then the difference in flags might only be the spelling of fmodule-file options, which are not hashed. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D132066/new/ https://reviews.llvm.org/D132066 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
