[PATCH] D133715: [ADT] Add TrieRawHashMap

Steven Wu via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Fri Jan 13 10:00:28 PST 2023


steven_wu added inline comments.


================
Comment at: llvm/unittests/ADT/HashMappedTrieTest.cpp:18
+namespace llvm {
+class HashMappedTrieTestHelper {
+public:
----------------
dblaikie wrote:
> steven_wu wrote:
> > dblaikie wrote:
> > > dexonsmith wrote:
> > > > steven_wu wrote:
> > > > > dblaikie wrote:
> > > > > > Rather than having to indirect everything through this class helper - I think it's possible to name the class of the test fixture itself, and then you could friend that, so the test fixture would have direct access? Might be simpler that way.
> > > > > I didn't see this comment.
> > > > > 
> > > > > Can you elaborate more how this work? Since friend doesn't inherit and `TEST_F` are subclasses of the test fixture, the best I can think is to create forwarding in the test fixture, then it is not that much different from what it is now. 
> > > > I think:
> > > > 
> > > > ```
> > > > lang=c++
> > > > namespace llvm::testing {
> > > > class HashMappedTrieTest;
> > > > }
> > > > 
> > > > namespace llvm {
> > > > class HashMappedTrie {
> > > >   friend class HashMappedTrieTest;
> > > > }
> > > > }
> > > > ```
> > > > And then use the `llvm::testing` namespace to implement the test. 
> > > > 
> > > https://stackoverflow.com/questions/2396370/how-to-make-google-test-classes-friends-with-my-classes seems to discuss how to friend a fixture directly, though may require gtest support/inclusion in the header.
> > I am not sure about adding friend class in library header that references tests (and for each tests added).
> > 
> > What do you think about the current implementation? There is an extra forwarding in the test helper but not too bad.
> It'd only be for the one test fixture here (HashMappedTrieTest), I think. Feels like it'd be nice to avoid the indirection through the `HashMappedTrieTestHelper`.
Sorry I still don't quite get it. Current `TrieRawHashMapTestHelper` is the only test fixture, but I have to put the indirection in there unless I want to friend every single tests (currently 3) in `llvm/ADT/TrieRawHashMap.h` using `FRIEND_TEST`. Am I missing something?


================
Comment at: llvm/unittests/ADT/TrieRawHashMapTest.cpp:293
+
+class MockTrieStringSet
+    : ThreadSafeTrieRawHashMap<std::string, sizeof(HashType)> {
----------------
dblaikie wrote:
> steven_wu wrote:
> > dblaikie wrote:
> > > This seems like a fair bit of text to wrap the trie - what's the value/extra test coverage this is providingg? (sorry I'm not following too clearly) I'd have hoped/expected this API could be tested more directly, without needing this wrapper and/or without needing to involve something as non-trivial as std::string (instead using simple user defined types in the test)?
> > This a simplified version of the original test from the original patch. The original patch implements a generic set data structure in this test file and tests how it trie can be used like a set. Now it is simplified to be a stringset with a fake hash algorithm because there is no current value to put TrieSet in ADT.
> What extra test coverage does it provide? How bad/what would be the tradeoff of testing that functionality more directly without the mock?
The extra test coverage it provides is the ability to store a non-POD type data into the Trie. where `insertLazy` is tested for lazy construction. I guess we can also tested it with the naive uint64_t.

The alternative is might be just put `ThreadSafeHashMappedTrieSet` from the original patch into a header in ADT but there isn't a use case for that yet. So maybe testing insertLazy with uint64_t will make this cleaner.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133715



More information about the llvm-commits mailing list