[PATCH] D92627: Basic: Add hashing support for FileEntryRef and DirectoryEntryRef

[Jan Svoboda via Phabricator via cfe-commits]

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