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
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
