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