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

[Adrian Prantl via Phabricator via llvm-commits]

aprantl added inline comments.


================
Comment at: lib/CodeGen/AsmPrinter/DwarfAccelTable.h:178
     DwarfStringPoolEntryRef Name;
-    std::vector<HashDataContents *> Values;
+    std::vector<DataT *> Values;
   };
----------------
JDevlieghere wrote:
> 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.
I'm mostly worried about code size. My understanding (please let me know if this is misinformed) is that by having the entire class be a template we will get N copies of all methods including the ones that don't depend on the templated type such as the constructor.
> No overhead of virtual calls.
Do you mean virtual calls when calling methods on the Values stored in the vector?
You could templateize the emit method itself and/or create a non-template AppleAccelTableBase class that contains all the generic code and templated AppleAccelTable<DataT> inheriting from that class. That's a common pattern I've seen used all over LLVM.


Repository:
  rL LLVM

https://reviews.llvm.org/D42334