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