[PATCH] D143414: [clang] refactor FileManager::GetUniqueIDMapping

[Richard Howell via Phabricator via cfe-commits]

rmaz added inline comments.


================
Comment at: clang/include/clang/Basic/FileManager.h:316
+  void getSeenFileEntries(
+      SmallVectorImpl<OptionalFileEntryRefDegradesToFileEntryPtr> &Entries)
+      const;
----------------
jansvoboda11 wrote:
> Since we're already modifying the two only users of this function, maybe we could use `Optional<FileEntryRef>` instead of `OptionalFileEntryRefDegradesToFileEntryPtr` (which we want to eventually remove)?
We could do that with less churn once more or the called methods in the loop over the files are refactored in D142724, but I'm fine with either approach.


================
Comment at: clang/lib/Basic/FileManager.cpp:625-627
+        Entries.push_back(
+            FileEntryRef(*reinterpret_cast<const FileEntryRef::MapEntry *>(
+                Value->V.get<const void *>())));
----------------
jansvoboda11 wrote:
> Why is this necessary? IIUC, `FileEntryRef` has this logic in `getBaseMapEntry()`. I would expect `FileEntryRef(Entry)` to be correct regardless of what's in `Value->V`.
I borrowed this from: https://github.com/llvm/llvm-project/blob/main/clang/lib/Basic/FileManager.cpp#L399-L408

I assumed this was necessary from @benlangmuir 's comment on D142780:

> Also, it will never give you a vfs mapped path since it's skipping those (V.dyn_cast<FileEntry *>()).

Are you saying we should just do something like:
```
for (const auto &Entry : SeenFileEntries) {
    if (llvm::ErrorOr<FileEntryRef::MapValue> Value = Entry.getValue())
        Entries.push_back(FileEntryRef(Entry));
}
```
?


================
Comment at: clang/lib/Serialization/ASTWriter.cpp:176
 
+  for (auto &File : FileEntries) {
     const HeaderFileInfo *HFI =
----------------
jansvoboda11 wrote:
> I'm afraid that iterating over file entries in "non-deterministic" order can cause non-determinism down the line. For example, calling `HeaderSearch::findAllModulesForHeader()` in the loop can trigger deserialization of `HeaderFileInfo` from loaded PCMs. That code fills the `Module::Headers` vectors, which we serialize without sorting first.
Do you think we should move the sorting into the `getSeenFileEntries()` method?


================
Comment at: clang/lib/Serialization/ASTWriter.cpp:1939
     // Massage the file path into an appropriate form.
     StringRef Filename = File->getName();
     SmallString<128> FilenameTmp(Filename);
----------------
jansvoboda11 wrote:
> This will insert duplicate entries (with the same key) into the on-disk hash table. I don't know if that's problematic, just thought I'd call it out.
Not sure I follow, why would we have `FileEntry`s with duplicate names?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143414