[PATCH] D42334: [NFC] Refactor Apple Accelerator Tables

[Jonas Devlieghere via Phabricator via llvm-commits]

JDevlieghere added a comment.

Thanks a lot for the prompt review Adrian!



================
Comment at: lib/CodeGen/AsmPrinter/DwarfAccelTable.h:178
     DwarfStringPoolEntryRef Name;
-    std::vector<HashDataContents *> Values;
+    std::vector<DataT *> Values;
   };
----------------
aprantl wrote:
> Reading just the struct definition I'm a little puzzled why this needs to be a template if we are only storing a pointer to DataT. i wonder if we could get away with just templateizing one or two methods instead of the entire class.
I don't think there's anything preventing this, I just liked this approach better because:

 1. No overhead of virtual calls.
 2. No risk of accidentally mixing different types of data in one table. (make interfaces easy to use correctly and hard to use incorrectly)
 3. No need to explicitly specify the atoms in the constructor and having it abstracted and self contained in the templated class. (same reason as 2)

Because of (2) and (3) it makes sense to have an `emit` interface where you call the appropriate methods to emit the data, because the atoms are right there and you have a guarantee about their form. If this is not the case, I think we should go for an interface similar to D42246 where DataT has an interface to get the data for a given atom and emit the correct form based on the atom's form; which means a loop and more function calls.

tl;dr it's an interface design decision and not a technical limitation.


================
Comment at: lib/CodeGen/AsmPrinter/DwarfAccelTable.h:288
+  unsigned index = 0;
+  for (size_t i = 0, e = Buckets.size(); i < e; ++i) {
+    Asm->OutStreamer->AddComment("Bucket " + Twine(i));
----------------
aprantl wrote:
> Could you convert this to LLVM style with capitalized variable names?
Sure! I left it like this based on Chandler's comment here: https://reviews.llvm.org/D40987?id=126062#inline-357702 


================
Comment at: lib/CodeGen/AsmPrinter/DwarfAccelTable.h:326
+  for (size_t i = 0, e = Buckets.size(); i < e; ++i) {
+    for (auto HI = Buckets[i].begin(), HE = Buckets[i].end(); HI != HE; ++HI) {
+      uint32_t HashValue = (*HI)->HashValue;
----------------
aprantl wrote:
> does this look better as a range-based-for?
Definitely. I removed most of them but this one slipped through the cracks. Thanks!


Repository:
  rL LLVM

https://reviews.llvm.org/D42334