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

[Ben Langmuir via Phabricator via cfe-commits]

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