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

[Jan Svoboda via Phabricator via cfe-commits]

jansvoboda11 added a comment.

I left a couple of smaller initial comments. I'd like to see this split into multiple patches. I can see some formatting changes, removal of `CompilerInvocation` from `ModuleDeps`, isolated changes to `Tooling`, etc. That'd make it much easier to review.



================
Comment at: clang/include/clang/Tooling/DependencyScanning/ModuleDepCollector.h:204
+  /// invocation, for example when there are multiple chained invocations.
+  void handleInvocation(CompilerInvocation CI);
+
----------------
Nit: `handle*()` functions are associated with `DependencyConsumer` in my brain. Could we find a distinct name to avoid confusion between all the different types we're using here?


================
Comment at: clang/include/clang/Tooling/DependencyScanning/ModuleDepCollector.h:224
+  /// a preprocessor. Storage owned by \c ModularDeps.
+  llvm::StringMap<ModuleDeps *> ModuleDepsByID;
+  /// Directly imported prebuilt deps.
----------------
I assume you're not using `llvm::DenseMap<ModuleID, ModuleDeps *>` because it's a hassle to implement for custom keys and performance is not a concern, correct? Any other aspects?


================
Comment at: clang/include/clang/Tooling/DependencyScanning/ModuleDepCollector.h:226
+  /// Directly imported prebuilt deps.
+  std::vector<PrebuiltModuleDep> DirectPrebuiltDeps;
+
----------------
Would this allow us to remove `ModuleDepCollectorPP::DirectPrebuiltModularDeps`?


================
Comment at: clang/lib/Tooling/DependencyScanning/ModuleDepCollector.cpp:358
+  for (auto &&I : DirectPrebuiltModularDeps) {
+    MDC.DirectPrebuiltDeps.emplace_back(I);
+    MDC.Consumer.handlePrebuiltModuleDependency(MDC.DirectPrebuiltDeps.back());
----------------
As said in a previous comment, we might avoid the copy by storing this in `MDC` in the first place.


================
Comment at: clang/lib/Tooling/DependencyScanning/ModuleDepCollector.cpp:471
+
+  MDC.setModuleDepsForID(MD.ID, &MD);
   return MD.ID;
----------------
Nit: this function might get easier to think about if we did this in one step with the context hash calculation:

```
MDC.associateWithContextHash(MD, CI);
MDC.addOutputPaths(MD, CI);
MD.BuildArguments = CI.getCC1CommandLine();
```


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

https://reviews.llvm.org/D132405