[PATCH] D133715: [ADT] Add HashMappedTrie

[David Blaikie via Phabricator via llvm-commits]

dblaikie added a comment.

Not sure I'll be the most detailed reviewer here, if anyone else wants to chime in.

I've a generally understanding of what a trie is, but at my current level of detail looking at this code it's not entirely clear to me what this particular data structure is - maybe some more comments both in the source and in the patch description? simple example usage (in a test and/or in those comments/patch description) migth be helpful



================
Comment at: llvm/include/llvm/ADT/HashMappedTrie.h:25
+public:
+  enum : size_t { TrieContentBaseSize = 4 };
+
----------------
I think we can use static constexpr implicitly inline constants now, rather than enums?


================
Comment at: llvm/include/llvm/ADT/HashMappedTrie.h:34-36
+  template <class T> static constexpr size_t getContentAllocSize() {
+    return sizeof(AllocValueType<T>);
+  }
----------------
Guess these could be constexpr variable templates rather than functions? But probably not a big deal either way.


================
Comment at: llvm/include/llvm/ADT/HashMappedTrie.h:63
+  public:
+    PointerBase() noexcept {}
+    PointerBase(PointerBase &&) = default;
----------------



================
Comment at: llvm/include/llvm/ADT/HashMappedTrie.h:156-160
+    static HashT makeHash(ArrayRef<uint8_t> HashRef) {
+      HashT Hash;
+      std::copy(HashRef.begin(), HashRef.end(), Hash.data());
+      return Hash;
+    }
----------------
This sort of feels a bit confusing - why are we hashing a hash?


================
Comment at: llvm/unittests/ADT/HashMappedTrieTest.cpp:71-75
+  // Dump out the trie so we can confirm the structure is correct. Each subtrie
+  // should have 2 slots. The root's index=0 should have the content for
+  // 0x37 directly, and index=1 should be a linked-list of subtries, finally
+  // ending with content for (max-2) and (max-3).
+  //
----------------
Seems unfortunate to do significant amounts of testing via the stringification of an object - any chance this could be done in a more API-centric way?


================
Comment at: llvm/unittests/ADT/HashMappedTrieTest.cpp:199
+    operator NumT() const { return Num; }
+    ~NumWithDestructorT() {}
+  };
----------------
This is specifically to make it non-trivially destructible? should this record the execution of the dtor and check that the right number (or even instance) of destructions occur?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133715