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