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

[Ben Langmuir via Phabricator via cfe-commits]

benlangmuir added inline comments.


================
Comment at: clang/include/clang/Tooling/DependencyScanning/DependencyScanningTool.h:34
+/// \see FullDependencies::Commands.
+class Command {
+public:
----------------
jansvoboda11 wrote:
> Have you considered using the `Job`/`Command` classes the driver uses? What are the downsides of doing that?
The `driver::Command`/`driver::JobList` is not standalone from the driver - it depends on an external `const Action &`, `const Tool &`, and a few `const char *`, so if we used it we would also need to preserve the `Compilation` and `Driver` they came from somewhere, or change  the API so the commands were only available inside a callback with limited scope that they are immediately copied out of.  I'm also not sure what the consequences of mutating Commands are (there is a `replaceArguments`, but I don't know if it could break invariants).


================
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)));
----------------
jansvoboda11 wrote:
> Shouldn't this be a responsibility of the dependency scanner?
Good question! I was mirroring how it works for modules where we do this in the `ModuleDepCollector` and store the whole invocation in the `ModuleDeps`.  But I guess that's "inside" the dep scanner, and this is "outside".  Happy to shuffle things around.

What is your opinion of  `takeFullDependencies()` adding to `FrontendOpts.ModuleFiles`, ? That is also arguably the scanner's responsibility.

Another thing we could do here is remove `BuildInvocation` from `FullDependencies` and `ModuleDeps` and only expose the serialized `-cc1` arguments.  It would simplify the new code a bit since there'd only be one kind of "command".   On the other hand, I could also see it being potentially useful to have the whole invocation available if you were writing a C++ tool that used the scanner API rather than just clang-scan-deps itself.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132405