[PATCH] D39111: Extensible LLVM RTTI

[Chandler Carruth via Phabricator via llvm-commits]

chandlerc added a comment.

In https://reviews.llvm.org/D39111#902702, @lhames wrote:

> > The virtual call could be removed, perhaps, by having the base class hold a non-static member void*...
>
> Oh that's clever. I hadn't thought of that. There is a space trade-off though as it would require one char per RTTI object (in addition to the current requirement of one BSS char per class).


I'm not sure... If there are other virtual functions, then there would need to be some other base class with that virtual interface? At that point, you get multiple inheritance, and it isn't clear this is a win.

This actually seems a bit hard to use interestingly with `RTTIRoot` and virtual methods.... Hmm...

Oh, I guess the pattern is that you have your base, abstract interface derive from RTTIRoot, and then you extend that abstract interface further using the CRTP RTTIExtends? I think having this spelled out a bit more in the comments would be helpful.

The interesting tradeoff here is that if the user is going to devirtualize their interface by using `dyn_cast` and `isa`, they might end up preferring a simpler mechanism using a data member in the base class because they wouldn't have a vptr. But if they *do* have a vptr, almost certainly want to use this to avoid growing the memory of the base class.

You could design this to be parameterizable and handle both cases, but not sure if you want to do that now or wait for a concrete use case...

> My current thinking is that open hierarchies are primarily (perhaps only?) useful for types that use virtual methods already, and RTTI calls are likely to be rare / non-performance-sensitive for such types. If it were accepted, my advice was going to be "Use the new system if you know you need an open hierarchy, or you know you don't care about performance".

My preference would be to focus *only* on open vs. closed. Maybe something along the lines of: "If at all possible, prefer a closed hierarchy rather than an open one, and use <technique> as it is faster and <reasons>. However, if you need an open, extensible type system use <technique> but be aware of <performance implications>."



================
Comment at: Support/ExtensibleRTTITest.cpp:21
+public:
+  static char ID;
+};
----------------
dblaikie wrote:
> These chars could be provided in the CRTP base, perhaps? Less boilerplate. (though this would mean they'd have linkonce linkage, rather than having strong external definitions - but I doubt that duplication in objects would be too painful?)
> 
> Could even be local to the RTTIExtends::classID function (& dynamicClassID would just call classID)
Sadly, this will (I suspect) run into the non-conforming implementation of static data members of class templates on MSVC between DLLs. Specifically, the address of ID will be different between two DLLs. =[

I'm actually a bit hesitant with this entire thing as a consequence. We need to really clearly document the hard constraints on how this CRTP base is used as a consequence of this (you cannot use this as part of class templates unless you explicitly instantiate all specializations of them).


================
Comment at: llvm/Support/ExtensibleRTTI.h:22
+
+/// Base class for error info classes. Do not extend this directly: Extend
+/// the RTTIExtends template subclass instead.
----------------
Stale comment referencing error info...

Actually, this type doesn't really make a lot of sense to me (see my larger comment above)


Repository:
  rL LLVM

https://reviews.llvm.org/D39111