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

[Ben Langmuir via Phabricator via cfe-commits]

benlangmuir added inline comments.


================
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.
----------------
jansvoboda11 wrote:
> 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?
No good reason.  I switched it to DenseMap and included the change in https://reviews.llvm.org/D132617


================
Comment at: clang/include/clang/Tooling/DependencyScanning/ModuleDepCollector.h:226
+  /// Directly imported prebuilt deps.
+  std::vector<PrebuiltModuleDep> DirectPrebuiltDeps;
+
----------------
jansvoboda11 wrote:
> Would this allow us to remove `ModuleDepCollectorPP::DirectPrebuiltModularDeps`?
I sunk it to MDC in https://reviews.llvm.org/D132617.  It needed to change to `MapVector` since we are also uniquing them.


================
Comment at: clang/lib/Tooling/DependencyScanning/ModuleDepCollector.cpp:358
+  for (auto &&I : DirectPrebuiltModularDeps) {
+    MDC.DirectPrebuiltDeps.emplace_back(I);
+    MDC.Consumer.handlePrebuiltModuleDependency(MDC.DirectPrebuiltDeps.back());
----------------
jansvoboda11 wrote:
> As said in a previous comment, we might avoid the copy by storing this in `MDC` in the first place.
We don't save any copies, since we need to actually store the `PrebuiltModuleDep` in `MDC` in order to apply it to a secondary CompilerInvocation after the preprocessor etc. is gone.  But I still like sinking it down.


================
Comment at: clang/lib/Tooling/DependencyScanning/ModuleDepCollector.cpp:471
+
+  MDC.setModuleDepsForID(MD.ID, &MD);
   return MD.ID;
----------------
jansvoboda11 wrote:
> 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();
> ```
Good idea, included in https://reviews.llvm.org/D132617


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

https://reviews.llvm.org/D132405