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

[Jan Svoboda via Phabricator via cfe-commits]

jansvoboda11 added inline comments.


================
Comment at: clang/include/clang/Basic/FileManager.h:316
+  void getSeenFileEntries(
+      SmallVectorImpl<OptionalFileEntryRefDegradesToFileEntryPtr> &Entries)
+      const;
----------------
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)?


================
Comment at: clang/lib/Basic/FileManager.cpp:625-627
+        Entries.push_back(
+            FileEntryRef(*reinterpret_cast<const FileEntryRef::MapEntry *>(
+                Value->V.get<const void *>())));
----------------
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`.


================
Comment at: clang/lib/Serialization/ASTWriter.cpp:176
 
+  for (auto &File : FileEntries) {
     const HeaderFileInfo *HFI =
----------------
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.


================
Comment at: clang/lib/Serialization/ASTWriter.cpp:1919-1922
+    if (A->getUID() == B->getUID())
+      return A->getName() < B->getName();
 
+    return A->getUID() < B->getUID();
----------------
Nit: I'd slightly prefer:

```
    if (A->getUID() != B->getUID())
      return A->getUID() < B->getUID();

    return A->getName() < B->getName();
    
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143414