[PATCH] D111560: [clang][modules] Cache loads of explicit modules imported by PCH

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith added inline comments.


================
Comment at: clang/lib/Tooling/DependencyScanning/DependencyScanningTool.cpp:41
   for (const PrebuiltModuleDep &PMD : PrebuiltModuleDeps) {
-    Args.push_back("-fmodule-file=" + PMD.ModuleName + "=" + PMD.PCMFile);
+    Args.push_back("-fmodule-file=" + PMD.PCMFile);
     Args.push_back("-fmodule-map-file=" + PMD.ModuleMapFile);
----------------
jansvoboda11 wrote:
> dexonsmith wrote:
> > Why drop the `<name>=` argument? Isn't it better to keep that information, rather than forcing the reader to crack open the file to know what's inside?
> I think it's nice to be consistent. We already pass PCM files in the `-fmodule-file=<path>` form for:
> * all dependencies when precompiling a module,
> * non-PCH dependencies dependencies when compiling a TU.
> The `-fmodule-file=<name>=<path>` format is handled lazily (done via header search), which might be less efficient (see 0f99d6a4413e3da6ec242c0ab715d6699045dea8). We don't need laziness, since we know we'll need to open the file anyways.
> 
> I'm open to revising this decision later as part of performance tuning, but for now, I'd like this to be consistent across the board.
> 
> Bonus points: it forces me to fix a Clang bug.
> 
> WDYT?
Okay, SGTM.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111560