This is an archive of the discontinued LLVM Phabricator instance.

[unittests] Add reverse iteration unit test for pointer-like keys
ClosedPublic

Authored by mgrang on Aug 28 2017, 6:12 PM.

Diff Detail

Repository
rL LLVM

Event Timeline

mgrang created this revision.Aug 28 2017, 6:12 PM
dblaikie added inline comments.Aug 29 2017, 12:55 PM
unittests/Support/ReverseIterationTest.cpp
87–92

I'm not sure this is going to be more stable than the previous version, right? The pointers may be strictly ordered now, but the hashing container might order them in other ways that could mean different orders depending on higher bits in the pointer values...

What I had in mind/was previously suggesting was to use integer keys that are pointer like & so you can control all the bits. This would work for the DenseMap test, presumably, but maybe not the SmallPtrSet test (which might only accept real pointer keys, not pointer-like keys).

99
llvm::reverse(IterKeys);
mgrang updated this revision to Diff 113678.Sep 3 2017, 12:00 AM

Added DenseMap test for pointer-like keys.

mgrang added inline comments.Sep 3 2017, 12:06 AM
unittests/Support/ReverseIterationTest.cpp
69

How about this implementation? PtrLikeInt is pointer-like but it hashes based on an integer value.
I don't think this would work for SmallPtrSet (as you had pointed out).

95

@dblaikie llvm::reverse(IterKeys) does not change the existing array, right?. I found using std::reverse cleaner than using another array to hold the reversed one. What do you say?

mgrang updated this revision to Diff 113719.Sep 3 2017, 9:19 PM

NFC. Made the static_assert the first statement in the test.

dblaikie added inline comments.Sep 5 2017, 8:59 AM
unittests/Support/ReverseIterationTest.cpp
61–67

Rather than mangling pointers - have two fixed instances of PtrLikeInt that can be pointed to (a local static PtrLikeInt in getEmpty/TombstoneKey would probably suffice:

static PtrLikeInt *getEmptyKey() {
  static const PtrLikeInt EmptyKey;
  return &EmptyKey;
}

something like that? (drop the 'inline' keyword from (static and non-static) member functions defined inline in a class - they already have that linkage)

70

Maybe just make the value /be/ the hash, do you think? Not sure if that makes it any better or worse (it doesn't have to be a good hash function - simplicity might be preferable)

Maybe make PtrLikeInt's member 'unsigned' to match the hash value type.

mgrang updated this revision to Diff 113900.Sep 5 2017, 12:50 PM
mgrang retitled this revision from [unittests] Add reverse iteration unit tests for pointer-like keys to [unittests] Add reverse iteration unit test for pointer-like keys.

Addressed comments.

mgrang marked 2 inline comments as done.Sep 5 2017, 12:53 PM
mgrang added inline comments.
unittests/Support/ReverseIterationTest.cpp
61–67

Making EmptyKey as a "const" would require an initializer as well as a change in the signature of the getEmptyKey function (which causes build conflicts due to mismatching definitions). So I have dropped the const.

dblaikie accepted this revision.Sep 5 2017, 1:21 PM

Looks good - thanks!

This revision is now accepted and ready to land.Sep 5 2017, 1:21 PM
This revision was automatically updated to reflect the committed changes.
mgrang marked an inline comment as done.