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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
