rsmith marked an inline comment as done.
rsmith added a comment.
In D54986#1312435 <https://reviews.llvm.org/D54986#1312435>, @lebedev.ri wrote:
> Will `-fforce-emit-vtables` still work?
Yes, it still works. (The tests for it still pass and I've manually tested it
too.)
================
Comment at: lib/AST/ASTContext.cpp:9801
+ RD->getTemplateSpecializationKind() ==
+ TSK_ExplicitInstantiationDefinition;
else
----------------
rjmccall wrote:
> Does it matter if it's not this particular declaration that's the explicit
> instantiation?
The explicit instantiation definition decl itself is the only one that CodeGen
needs to see. (Earlier declarations would have a different TSK, and the
language rules generally don't permit any later decls.)
================
Comment at: lib/AST/RecordLayoutBuilder.cpp:3069
const Decl *Result =
Entry ? Entry.get(getExternalSource()) : computeKeyFunction(*this, RD);
----------------
rjmccall wrote:
> Why not just change `computeKeyFunction` to decide that the class doesn't
> have a key function if it would be an inline definition? We can do the same
> updating we do with the ARM rule except just bailing out instead of scanning
> for another candidate.
It seems desirable to me for Clang's notion of what the "key function" is to
exactly match the ABI specification. That way, the decision to not emit a
vtable with an inline key function is purely a lowering concern, not part of
the program model represented by the frontend, and non-compiler tools who query
this data will get correct answers. Also, if we made the suggested change, I'd
still want to perform the linkage check to avoid unnecessary emission of
vtables for explicit instantiation definitions, so this wouldn't remove any
complexity from CodeGen.
If we want to go down this path, I'd prefer that we also change the ABI
document to say that an inline virtual function definition results in the class
having no key function.
================
Comment at: lib/CodeGen/CGVTables.cpp:889
+ if (!llvm::GlobalValue::isDiscardableIfUnused(getVTableLinkage(theClass)))
+ getCXXABI().emitVTables(theClass);
}
----------------
rjmccall wrote:
> I feel like "don't emit the v-table for this" should be handled in the caller.
OK, but that'd mean putting the same check into both callers of this function.
As an alternative, what do you think about renaming this function to something
more like "MarkVTableDemanded" that doesn't suggest that we'll necessarily emit
it?
Repository:
rC Clang
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D54986/new/
https://reviews.llvm.org/D54986
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
