[PATCH] D133715: [ADT] Add HashMappedTrie

[David Blaikie via Phabricator via llvm-commits]

dblaikie added a comment.

In D133715#4023734 <https://reviews.llvm.org/D133715#4023734>, @steven_wu wrote:

> 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.

The ability to dump the data structure I'm all for - like DenseMap, etc, but I think that dumping should be the user-visible state (it's a map, the trie part is an implementation detail that ideally shouldn't be exposed in the dump developers use regularly - presumably it's not necessary except when debugging the data structure itself, which should be rare (we don't usually need to look at DenseMap's internal bucketing, for instance)).

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



In D133715#4023992 <https://reviews.llvm.org/D133715#4023992>, @steven_wu wrote:

> Address review feedback and rebase the patch using std::optional
>
> Still thinking how to unittest without string compare. The only way I can think is to implement iteration on HashMappedTrie so we can have API to check the state of SubTrie. That will expose more types to user, which will make API more complicated.

Can the unit test be a friend to the data structure and use the data structure's internal APIs?

> Also for discussion of names, do we have other candidates to replace "HashMappedTrie" that is better and clearer?

I'd really like to avoid "trie" in the name, though I guess it's as much an implementation detail as "Dense" is in DenseMap, so might be deserving to be in the name.

I think this is a map, so the name should probably end in 'map' not in 'trie'. I'd guess `TrieHashMap` - it's a type of hash map that uses a trie? Maybe make a shortlist of names to consider, etc?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133715