[PATCH] D133715: [ADT] Add TrieRawHashMap

David Blaikie via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Tue Jan 10 12:32:39 PST 2023


dblaikie added inline comments.


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


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


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