[PATCH] D132405: [clang][deps] Split translation units into individual -cc1 or other commands

[Jan Svoboda via Phabricator via cfe-commits]

jansvoboda11 added inline comments.


================
Comment at: clang/include/clang/Tooling/DependencyScanning/DependencyScanningTool.h:34
+/// \see FullDependencies::Commands.
+class Command {
+public:
----------------
Have you considered using the `Job`/`Command` classes the driver uses? What are the downsides of doing that?


================
Comment at: clang/include/clang/Tooling/DependencyScanning/DependencyScanningWorker.h:37
+                                     std::vector<std::string> Args) = 0;
+  virtual void handleInvocation(clang::CompilerInvocation CI) = 0;
+
----------------
Is `clang::` necessary here?


================
Comment at: clang/include/clang/Tooling/DependencyScanning/DependencyScanningWorker.h:102
+std::vector<std::string>
+serializeCompilerInvocation(const CompilerInvocation &CI);
+
----------------
Maybe we could add this API directly to `CompilerInvocation`. I can see lots of clients not caring about the (potentially more efficient) `CompilerInvocation::generateCC1CommandLine()` with `StringAllocator` and inventing their own implementation of this.


================
Comment at: clang/lib/Tooling/DependencyScanning/DependencyScanningTool.cpp:155
+void FullDependencyConsumer::handleInvocation(CompilerInvocation CI) {
+  clearImplicitModuleBuildOptions(CI);
+  Commands.push_back(std::make_unique<CC1Command>(std::move(CI)));
----------------
Shouldn't this be a responsibility of the dependency scanner?


================
Comment at: clang/lib/Tooling/DependencyScanning/DependencyScanningTool.cpp:197
+        for (const auto &ID : FD.ClangModuleDeps)
+          FrontendOpts.ModuleFiles.push_back(
+              LookupModuleOutput(ID, ModuleOutputKind::ModuleFile));
----------------
Just a heads-up: after I land D132066 today, you might need to update this to respect the eager/lazy loading mode.


================
Comment at: clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp:438
+
+void dependencies::clearImplicitModuleBuildOptions(CompilerInvocation &CI) {
+  CI.getLangOpts()->ImplicitModules = false;
----------------
Wondering if we could move this to `CompilerInvocation`, right besides `resetNonModularOptions()`.


================
Comment at: clang/test/ClangScanDeps/diagnostics.c:31
 // CHECK-NEXT:     {
-// CHECK-NEXT:       "clang-context-hash": "[[HASH_TU:.*]],
+// CHECK:            "clang-context-hash": "[[HASH_TU:.*]],
 // CHECK-NEXT:       "clang-module-deps": [
----------------
benlangmuir wrote:
> I didn't want to radically change all the tests, so in most cases I just loosened the matching so it would match inside the "commands".  The modules-full.c and multiple-commands.c cover the whole structure in detail.
That sounds good to me.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132405