[PATCH] D157066: [clang][modules][deps] Create more efficient API for visitation of `ModuleFile` inputs

[Jan Svoboda via Phabricator via cfe-commits]

jansvoboda11 added inline comments.


================
Comment at: clang/include/clang/Serialization/ModuleFile.h:72
+  bool TopLevel;
+  bool ModuleMap;
 };
----------------
benlangmuir wrote:
> Is there something using this new split? It seems like every user is using `&&` for these
You're right. I recall needing this on (now probably abandoned) patch, so I thought I might as well add the extra bit now, sine I'm changing the format anyways. I'm fine with removing this.


================
Comment at: clang/lib/Serialization/ASTReader.cpp:2411
   bool Transient = FI.Transient;
-  StringRef Filename = FI.Filename;
+  StringRef Filename = FI.FilenameAsRequested;
   uint64_t StoredContentHash = FI.ContentHash;
----------------
benlangmuir wrote:
> It's not clear to me why this one changed
This actually maintains the same semantics - `FI.Filename` was previously the as-requested path, now it's the on-disk path. Without changing this line, `FileManager::getFileRef()` would get called with the on-disk path, meaning consumers of this function would get the incorrect path when calling `FileEntryRef::getNameAsRequested()` on the returned file. I recall one clang-scan-deps test failing because of it.


================
Comment at: clang/lib/Serialization/ASTReader.cpp:9323
 
+void ASTReader::visitInputFileInfos(
+    serialization::ModuleFile &MF, bool IncludeSystem,
----------------
benlangmuir wrote:
> Can we rewrite `visitInputFiles` on top of this?
I can try consolidating these in a follow-up if you're fine with that.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157066