[PATCH] D133715: [ADT] Add TrieRawHashMap

[Steven Wu via Phabricator via llvm-commits]

steven_wu added inline comments.


================
Comment at: llvm/unittests/ADT/HashMappedTrieTest.cpp:18
+namespace llvm {
+class HashMappedTrieTestHelper {
+public:
----------------
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.


================
Comment at: llvm/unittests/ADT/TrieRawHashMapTest.cpp:293
+
+class MockTrieStringSet
+    : ThreadSafeTrieRawHashMap<std::string, sizeof(HashType)> {
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133715