[PATCH] D106876: Remove non-affecting module maps from PCM files.

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added a comment.

This should have a testcase. I would imagine you can test this by constructing a situation where you build a module twice with different sets of module map files being considered and checking that you get bit-for-bit identical outputs. (For example: set up a temporary area for the test case files, build a module there, then copy in an irrelevant module map file into a directory named by a `-I` and build the module again.)



================
Comment at: clang/lib/Serialization/ASTWriter.cpp:164
+
+std::set<std::string> GetAllModulemaps(const HeaderSearch &HS,
+                                       Module *RootModule) {
----------------
`Modulemap` -> `ModuleMap` for consistency please.


================
Comment at: clang/lib/Serialization/ASTWriter.cpp:168
+  std::set<Module *> ProcessedModules;
+  std::set<Module *> ModulesToProcess{RootModule};
+
----------------
You're walking this list in a non-deterministic order; consider using a `SmallVector` here and popping elements from the end instead of the front in the loop below (ie, depth-first traversal instead of breadth-first).


================
Comment at: clang/lib/Serialization/ASTWriter.cpp:183-184
+        HS.getExistingFileInfo(File, /*WantExternal*/false);
+    if (!HFI || (HFI->isModuleHeader && !HFI->isCompilingModuleHeader))
+      continue;
+
----------------
I don't think this is right: I think we should consider every file that we've entered in this compilation, regardless of whether it's part of a module we're compiling. (We support modes where we'll enter modular headers despite not compiling them.) Can you replace this with just `if (!HFI) continue;` and still get the optimization you're looking for?


================
Comment at: clang/lib/Serialization/ASTWriter.cpp:186
+
+    const auto KH = HS.findModuleForHeader(File, /*AllowTextual*/true);
+    if (!KH.getModule())
----------------
This should be `findAllModulesForHeader`: in the case where there is more than one module covering a header, the additional module maps can matter in some contexts.


================
Comment at: clang/lib/Serialization/ASTWriter.cpp:1537-1538
+        !AffectingModuleMaps.empty() &&
+        AffectingModuleMaps.find(std::string(Cache->OrigEntry->getName())) ==
+            AffectingModuleMaps.end()) {
+      // Do not emit modulemaps that do not affect current module.
----------------
This will not work correctly if a module map file is reachable via multiple different paths. Consider using `FileEntry*` comparisons instead here.


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

https://reviews.llvm.org/D106876