[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/DependencyScanningWorker.h:27
+namespace driver {
+class Command;
+}
----------------
Not needed anymore, I assume.


================
Comment at: clang/include/clang/Tooling/DependencyScanning/DependencyScanningWorker.h:39
 
+  virtual void handleBuildCommand(std::string Executable,
+                                  std::vector<std::string> Args) = 0;
----------------
Would it make sense to accept the `clang::tooling::dependencies::Command` struct here?


================
Comment at: clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp:165
 
+    if (Scanned) {
+      // If we have already scanned an upstream command, just forward to the
----------------
This makes sure we only run scan once per driver invocation? Can you expand on this a bit? Maybe even put the reasoning into a comment in the code.


================
Comment at: clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp:444
+              Invocation.setDiagnosticOptions(&Diags->getDiagnosticOptions());
+              return Invocation.run();
+            });
----------------
I'm not particularly fond of the fact that `Consumer.handleBuildCommand()` is called in this lambda directly in the non-clang case, but several objects deep in the normal case (`ToolInvocation` -> `DependencyScanningAction`). I think a clearer way to do this would be to somehow extract the `CompilerInvocation` (or `Command`) result from `ToolInvocation` and report it in this lambda too.


================
Comment at: clang/lib/Tooling/DependencyScanning/ModuleDepCollector.cpp:177
 
+static bool needsModules(FrontendInputFile FIF) {
+  switch (FIF.getKind().getLanguage()) {
----------------
I think this could be useful for other tools too in the future. Do you think it would make sense to put this in a more prominent header, so that other people can find it and reuse it more easily?


================
Comment at: clang/test/ClangScanDeps/deprecated-driver-api.c:1
+// RUN: rm -rf %t
+// RUN: split-file %s %t
----------------
Please summarize what this test does.


================
Comment at: clang/test/ClangScanDeps/multiple-commands.c:1
+// RUN: rm -rf %t
+// RUN: split-file %s %t
----------------
Please summarize what this test does.


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

https://reviews.llvm.org/D132405