[PATCH] D133715: [ADT] Add HashMappedTrie

[Steven Wu via Phabricator via llvm-commits]

steven_wu added a comment.

In D133715#3996263 <https://reviews.llvm.org/D133715#3996263>, @dblaikie wrote:

> In D133715#3996194 <https://reviews.llvm.org/D133715#3996194>, @dexonsmith wrote:
>
>> In D133715#3996103 <https://reviews.llvm.org/D133715#3996103>, @dblaikie wrote:
>>
>>> eh, maybe string's easier to read in expect diagnostics anyway... just seems a bit awkward/circuitous/unfortunate :/ (I guess the stringification could move into the test code, implemented via such a friend relationship)
>>
>> I do remember the string being easy to read in expect diagnostics, FWIW.
>>
>> What about renaming the methods to `printLayout()` and `dumpLayout()`? Then `print()` and `dump()` would at least still be available for something user-centric.
>
> Workable, though I'd still rather it be moved to the test file if it's not too inconvenient (with some friendship, probably). Avoid muddying the implementation with test-only features.

I find stringfication quite useful for debugging or inspecting the state of the CAS. Our branch (to be upstream later) has the OnDiskCAS, and we have a tool that can dump the CAS state (mostly the Trie information) in string format when pointed to the on disk CAS. My point is that even it is a testing/debugging tool, it is not limited to unit test only. I am ok with either names.

But I think we can expose some private methods for debugging to avoid string comparison in unit test and hopefully that is more readable.



================
Comment at: llvm/unittests/ADT/HashMappedTrieTest.cpp:241-245
+using HasherT = SHA1;
+using HashType = decltype(HasherT::hash(std::declval<ArrayRef<uint8_t> &>()));
+template <class T>
+class ThreadSafeHashMappedTrieSet
+    : ThreadSafeHashMappedTrie<T, sizeof(HashType)> {
----------------
dblaikie wrote:
> dexonsmith wrote:
> > FYI, here, buried in the unit tests (demoted due to lack of immediate use cases), this is a set data structure built on top of the trie.
> > 
> > If this were lifted up again for actual use, as opposed to just a test, you'd want to template it on `HasherT` and then use HashBuilder (from llvm/include/llvm/Support/HashBuilder.h) to hash the value.
> Hmm, yeah, this looks like overkill for the test - given the data structure as-is, we don't need any hash algorithm here - the test, I would expect, would use unique array values/"hash" values hardcoded/directly.
> 
> If you've got a use for this data structure, it could come along in a subsequent patch?
Agree. I don't think the actual data structure is useful outside the context of this unit test. I will simply it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133715