[PATCH] D112915: [clang][modules] Track included files per submodule

[Volodymyr Sapsai via Phabricator via cfe-commits]

vsapsai added inline comments.


================
Comment at: clang/include/clang/Lex/Preprocessor.h:1251-1260
+  const IncludedFilesSet *getNullSubmoduleIncludes() const {
+    auto It = IncludedFilesPerSubmodule.find(nullptr);
+    return It == IncludedFilesPerSubmodule.end() ? nullptr : &It->second;
   }
 
-  /// Get the set of included files.
-  IncludedFilesSet &getIncludedFiles() { return IncludedFiles; }
-  const IncludedFilesSet &getIncludedFiles() const { return IncludedFiles; }
+  /// Get the set of files included in the given (sub)module.
+  const IncludedFilesSet *getLocalSubmoduleIncludes(Module *M) const {
----------------
Was curious why `getNullSubmoduleIncludes` isn't `getLocalSubmoduleIncludes(nullptr)`? If we want to have separate methods and implementations, it might be useful to assert `M` isn't null in `getLocalSubmoduleIncludes`.


================
Comment at: clang/lib/Serialization/ASTReader.cpp:8631
 
+const Preprocessor::IncludedFilesSet *ASTReader::getIncludedFiles(Module *M) {
+  ModuleFile *F = getModuleManager().lookup(M->getASTFile());
----------------
Can you please check again the returned pointer doesn't end up as a dangling pointer? I don't think we store the pointer anywhere, which is good. My bigger concern is if we can invalidate `SubmoduleIncludedFiles` iterator while working with the returned pointer. I haven't found any indication of that but would like somebody else to check that.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112915