[PATCH] D33437: Emit available_externally vtables opportunistically

[John McCall via Phabricator via cfe-commits]

rjmccall 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());
----------------
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.


================
Comment at: include/clang/AST/VTableBuilder.h:169
+      return GlobalDecl(DtorDecl, CXXDtorType::Dtor_Deleting);
+    default:
+      llvm_unreachable("Only function pointers kinds");
----------------
By "exhaustive" I mean that you should list out all the cases instead of using default.  It means that someone who adds a new kind of v-table entry will get alerted to fix this switch.  There's only five other cases, it's not too bad.


https://reviews.llvm.org/D33437