[PATCH] D39111: Extensible LLVM RTTI

[Chandler Carruth via Phabricator via llvm-commits]

chandlerc added inline comments.


================
Comment at: Support/ExtensibleRTTITest.cpp:21
+public:
+  static char ID;
+};
----------------
lhames wrote:
> chandlerc wrote:
> > 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).
> > 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 not familiar with this. Is it only a problem for static data members on class templates, or does it apply to static members of ordinary classes too?
> 
> In this case MyBaseClass is an ordinary class, so I would have hoped it would be safe.
> 
> I had originally defined ID in the RTTIExtends class template so the user didn't have to write anything at all, but ran in to an issue (similar to the one you described) with MinGW, so abandoned that idea.
> 
Only an issue for static data members on class templates.

So, as long as the types providing the IDs are non-templates and you're getting the correct one's address via a virtual function call, this should work out fine IIRC. But moving them into the CRTP base class won't work. =/


Repository:
  rL LLVM

https://reviews.llvm.org/D39111