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

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith added inline comments.


================
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();
----------------
jansvoboda11 wrote:
> 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`.
Yes, that seems decent; I hadn't considered using `isSameRef` when the `MapEntry` is a bogus pointer, but that's probably better too. I'll update this and the instance of that below.


================
Comment at: clang/unittests/Basic/FileEntryTest.cpp:126
 
+TEST(FileEntryTest, DenseMapInfo) {
+  RefMaps Refs;
----------------
jansvoboda11 wrote:
> Question: do we prefer naming tests with the function/data structure under test, or using more descriptive names? (e.g. `DenseMapFindsOldestEntry`)
Likely depends on the primary motivation of the test; I have a bit of tendency for the former in case expected behaviour changes, unless the whole point of the test is to test an interesting behaviour or corner case.

In this case, the main purpose of the test is to demonstrate that these can be used as DenseMap-family keys -- i.e., that the `insert` calls successfully compile. It's important that the behaviour is correct, too, of course.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92627