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

[Ben Langmuir via Phabricator via cfe-commits]

benlangmuir marked 11 inline comments as done.
benlangmuir added inline comments.


================
Comment at: clang/include/clang/Tooling/DependencyScanning/DependencyScanningTool.h:44
+
+  enum CommandKind {
+    CK_CC1,
----------------
tschuett wrote:
> Why is this not an enum class?
Removed in latest change.


================
Comment at: clang/include/clang/Tooling/DependencyScanning/DependencyScanningTool.h:80
+
+  static bool classof(const Command *C) { return C->getKind() == CK_Simple; }
+};
----------------
tschuett wrote:
> Why are all members public?
Latest change simplifies this to a struct.


================
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:
> benlangmuir wrote:
> > 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.
> > 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.
> As discussed offline, I'd prefer keeping `CompilerInvocation` internal to the dependency scanner and exposing simple types (`std::vector<std::string>` for the command line) unless clients need more flexibility.
> 
> > What is your opinion of `takeFullDependencies()` adding to `FrontendOpts.ModuleFiles`, ? That is also arguably the scanner's responsibility.
> I'd prefer adding to `FrontendOpts.ModuleFiles` earlier, before the consumer.
> 
> > 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.
> +1
Done in latest changes. The logic for mutating the compiler invocation is now contained in the ModuleDepCollector, alongside the existing logic for doing the same kind of changes to module build commands.


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

https://reviews.llvm.org/D132405