jansvoboda11 added inline comments.
================
Comment at:
clang/include/clang/Tooling/DependencyScanning/DependencyScanningTool.h:34
+/// \see FullDependencies::Commands.
+class Command {
+public:
----------------
benlangmuir wrote:
> 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).
Makes sense, let's keep this as-is then.
================
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)));
----------------
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
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D132405/new/
https://reviews.llvm.org/D132405
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
