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

Wed Jan 24 02:10:07 PST 2018

JDevlieghere planned changes to this revision.
JDevlieghere added a comment.

In https://reviews.llvm.org/D42334#986126, @labath wrote:

> I find the fact that you have to specify type of data in the addName call a bit weird. This means you cannot guarantee the homogeneity of the table, as different addName calls on the same table may use different types (and none of them needs to match the atom array you passed to the constructor). It sounds to me like the data type should really a part of the table type (but you have not done that because of the code size concerns et al.).

Exactly.

> This "non-template base class" idea has already been proposed earlier in the thread by Adrian, but got dismissed because at that point you were still trying to optimize everything for speed.

Correct, because by templating the data type we don't need a virtual call.

> Now that you already have a base class for the data types, it should be fairly trivial to implement: put everything except the constructor and the addName function in the base class; the templated constructor can implicitly take the atom array as T::Atoms, and addName will automatically construct entries of type T. That should guarantee consistency with only a tiny code size increase.
> 
> What do you think?

I think it's a great idea! This would allow us to regain the strong interface I was aiming for.

https://reviews.llvm.org/D42334