[PATCH] D33437: Emit available_externally vtables opportunistically

[Piotr Padlewski via Phabricator via cfe-commits]

Prazek marked 4 inline comments as done.
Prazek added inline comments.


================
Comment at: include/clang/AST/VTableBuilder.h:160
+           "GlobalDecl can be created only from virtual function");
+    if (getKind() == CK_FunctionPointer)
+      return GlobalDecl(getFunctionDecl());
----------------
rjmccall wrote:
> Prazek wrote:
> > rjmccall wrote:
> > > Please use an exhaustive switch.  You can put llvm_unreachable in the other cases.
> > Should I implement this for RTTI fields? Or maybe leave a fixme comment that it could also work for some other fields in vtable, but is not currently used?
> I think your precondition of isUsedFunctionPointerKind() is fine, you don't need to handle RTTI in this function.
> 
> This does raise the interesting question, though, of whether this approach is safe for lazily-emitted RTTI.  I guess it currently works because IRGen doesn't use the normal deferred-emission mechanism for RTTI objects, so if the RTTI object is lazily-emitted, we'll just eagerly emit it instead of potentially ending up with an illegal reference to external RTTI.  You should add a comment to the appropriate place in CGRTTI (i.e. where we fill in the global definition) saying that this optimization may need adjustment if we ever switch to using deferred emission for some reason.
I couldn't find CGRTTI, so I added comment in Itanium and Microsoft ABI where we create RTTI fields, but maybe the CodeGenModule::GetAddrOfRTTIDescriptor would be a better place instead?

I also added small note to the EmitVTablesOpportunistically about this.


https://reviews.llvm.org/D33437