benlangmuir marked 6 inline comments as done.
benlangmuir added inline comments.
================
Comment at:
clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp:165
+ if (Scanned) {
+ // If we have already scanned an upstream command, just forward to the
----------------
jansvoboda11 wrote:
> 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.
In theory you want to scan once for each independent chain of -cc1 commands,
but since we don't yet support multi-arch builds in the scanner that just means
scan once per driver invocation.
I'll add a comment.
================
Comment at:
clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp:444
+ Invocation.setDiagnosticOptions(&Diags->getDiagnosticOptions());
+ return Invocation.run();
+ });
----------------
jansvoboda11 wrote:
> 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.
Yeah, this area has gone through a lot of churn as I try to balance the desire
to keep it clear where the consumer should be called vs. trying to keep the MDC
contained to the action.
I took another crack at it in the latest diff.
================
Comment at: clang/lib/Tooling/DependencyScanning/ModuleDepCollector.cpp:177
+static bool needsModules(FrontendInputFile FIF) {
+ switch (FIF.getKind().getLanguage()) {
----------------
jansvoboda11 wrote:
> 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?
I would prefer not to expose this without more understanding of what other use
cases there are. It seems like there are many ways to interpret "needsModules"
-- most of the time you probably want something more like
`LangOptions::Modules`.
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
