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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
