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

_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to