[PATCH] D43285: [CodeGen] Refactor AppleAccelTable

[Pavel Labath via Phabricator via llvm-commits]

labath added inline comments.


================
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
----------------
JDevlieghere wrote:
> 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.  
It's customary (at least in some parts of the llvm codebase) to place the things which have to be declared in the header, but that are not really considered to be a public interface into the `detail` namespace. I've put it here because I don't think anyone on the outside should ever refer to this non-templated class directly as doing so (particularly in combination with the non-templated version of emitAppleAccelTable) would allow you to circumvent some of the type safety checks (and probably crash).

But it seems that's not how things are done here (this is the only instance of the detail namespace in CodeGen), so I've removed 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())) {}
----------------
JDevlieghere wrote:
> Is there any advantage over passing in the hash value directly?
We delay computing the hash in the `try_emplace` function (in `AddName`) until we know that we really need to construct a new HashData object (i.e., that this is a new name).


================
Comment at: include/llvm/CodeGen/AccelTable.h:150
 
-  using HashList = std::vector<HashData *>;
-  HashList Hashes;
+  HashFn *Hash;
+  uint32_t BucketCount;
----------------
JDevlieghere wrote:
> `std::function<uint32_t(StringRef)>`?
I think we should avoid that as `std::function` is fairly heavy (it uses virtual dispatch and what-not). Besides, in the current model, we obtain the hash function as `&DataT::hash`, so this cannot really be anything other than a simple function pointer.


================
Comment at: lib/CodeGen/AsmPrinter/AccelTable.cpp:91
+/// Represents the emission state of a single accelerator table.
+class EmissionContext {
+protected:
----------------
JDevlieghere wrote:
> [opinion] I think `AccelTableEmitter` or something like that would better convey the purpose of this class.
I agree. (It's called this way because this started out just as a plain holder for the various bits needed for serialization).


Repository:
  rL LLVM

https://reviews.llvm.org/D43285