[PATCH] D133715: [ADT] Add HashMappedTrie

[David Blaikie via Phabricator via llvm-commits]

dblaikie added a comment.

In D133715#3993416 <https://reviews.llvm.org/D133715#3993416>, @dexonsmith wrote:

> In D133715#3993311 <https://reviews.llvm.org/D133715#3993311>, @dblaikie wrote:
>
>>> Note that there's an example data structure called ThreadSafeHashMappedTrieSet buried in the unit tests, which has an interface that hides the hash. Could be useful for some clients.
>>>
>>> - Maybe it would make sense to lift up? Name could be PerfectHashSet
>>> - You might want to build a map that also handles the hashing; name could potentially be PerfectHashMap
>>>
>>> Maybe this could be PerfectHashSetImpl?
>>>
>>> - Has the "guts" for implementing a client-friendly PerfectHashSet or PerfectHashMap?
>>> - ... but can be used directly if you want to manage the hashing yourself?
>>
>> I take it the current use cases you have in mind/lined up in the CAS use this directly? Maybe a `Raw` prefix? `RawPerfectHashSet`, `RawConcurrentHashSet`, etc... some combination/choice of those sort of things?
>
> @steven_wu can confirm, but that's what I remember. (There was an early branch that used the hash-included-client-friendly set, but the use case for it was refactored IIRC.)
>
> To summarize, seems like the patch needs:
>
> - a new name (`RawConcurrentHashSet` SGTM);

+1

> - some header docs explaining how to use it / design motivation;

+1

> - a test support friend class that can inspect/test the trie layout without relying on stringification;

This bit I'm not so sure about - we don't test the bucketing behavior of `DenseHashMap` so far as I know (or other similar implementation details of the various other hashing data structures - (since they're the ones with fairly complicated implementation details, they seem the best comparison)), for instance, we only test its interface - why would we do differently for this data structure?

> - unit tests rewritten to use the friend (and probably dropping the example higher-level data structure).

+1


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133715