[PATCH] D43285: [CodeGen] Refactor AppleAccelTable

[Jonas Devlieghere via Phabricator via llvm-commits]

JDevlieghere added a comment.

Looks good, with a few minor comments inline.

I think it would be great if we can add a small diagram or textual explanation to the top of the header file with the way the classes are structure, similar to your summary here. The comments on the individual classes are clear enough imho, but I can imagine that the big picture is a little harder to get when you don't know why we have all these different classes.



================
Comment at: include/llvm/CodeGen/AccelTable.h:117
 
-/// Apple-style accelerator table base class.
-class AppleAccelTableBase {
-protected:
+namespace detail {
+/// A base class holding non-template-dependant functionality of the AccelTable
----------------
What was your motivation for wrapping this into a separate namespace? When I saw `detail` I assumed this is where the differences between the Apple and DWARF tables would live. Maybe I'm missing something  but otherwise I think we'd be better of without it.  


================
Comment at: include/llvm/CodeGen/AccelTable.h:132
 
-    HashData(DwarfStringPoolEntryRef Name) : Name(Name) {
-      HashValue = djbHash(Name.getString());
-    }
+    HashData(DwarfStringPoolEntryRef Name, HashFn *Hash)
+        : Name(Name), HashValue(Hash(Name.getString())) {}
----------------
Is there any advantage over passing in the hash value directly?


================
Comment at: include/llvm/CodeGen/AccelTable.h:150
 
-  using HashList = std::vector<HashData *>;
-  HashList Hashes;
+  HashFn *Hash;
+  uint32_t BucketCount;
----------------
`std::function<uint32_t(StringRef)>`?


================
Comment at: lib/CodeGen/AsmPrinter/AccelTable.cpp:91
+/// Represents the emission state of a single accelerator table.
+class EmissionContext {
+protected:
----------------
[opinion] I think `AccelTableEmitter` or something like that would better convey the purpose of this class.


Repository:
  rL LLVM

https://reviews.llvm.org/D43285