jansvoboda11 accepted this revision.
jansvoboda11 added a comment.
This revision is now accepted and ready to land.
Left a couple of nits and a question, but LGTM overall.
================
Comment at: clang/include/clang/Basic/DirectoryEntry.h:104
+ bool isSpecialDenseMapKey() const {
+ return ME == llvm::DenseMapInfo<const MapEntry *>::getEmptyKey() ||
+ ME == llvm::DenseMapInfo<const MapEntry *>::getTombstoneKey();
----------------
Nit: would it make sense to avoid copy-pasting the constructor logic here
(`llvm::DenseMapInfo<const MapEntry *>::getEmptyKey()`) and call the
constructor instead?
For example: `isSameRef(DirectoryEntryRef(dense_map_empty_tag{}))`.
The same goes for `FileEntryRef`.
================
Comment at: clang/include/clang/Basic/DirectoryEntry.h:212
+ // Catch the easy cases: both empty, both tombstone, or the same ref.
+ if (LHS.ME == RHS.ME)
+ return true;
----------------
Nit: could use `LHS.isSameRef(RHS)` to keep this uniform.
================
Comment at: clang/unittests/Basic/FileEntryTest.cpp:126
+TEST(FileEntryTest, DenseMapInfo) {
+ RefMaps Refs;
----------------
Question: do we prefer naming tests with the function/data structure under
test, or using more descriptive names? (e.g. `DenseMapFindsOldestEntry`)
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D92627/new/
https://reviews.llvm.org/D92627
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
