================
@@ -496,17 +496,18 @@ bool
SemaCUDA::inferTargetForImplicitSpecialMember(CXXRecordDecl *ClassDecl,
// Look for special members in base classes that should be invoked from here.
// Infer the target of this member base on the ones it should call.
- // Skip direct and indirect virtual bases for abstract classes.
+ // Skip direct and indirect virtual bases for abstract classes, except for
+ // destructors — the complete destructor variant destroys virtual bases
+ // regardless of whether the class is abstract.
llvm::SmallVector<const CXXBaseSpecifier *, 16> Bases;
for (const auto &B : ClassDecl->bases()) {
if (!B.isVirtual()) {
Bases.push_back(&B);
}
}
- if (!ClassDecl->isAbstract()) {
+ if (!ClassDecl->isAbstract() || CSM == CXXSpecialMemberKind::Destructor)
----------------
Artem-B wrote:
I wonder what was the reason we're not appending abstract classes to the base
list?
If we do append some of them now, then:
- will it trigger the problem the check was intended to guard against.
- if adding abstract classes is safe, should we add all of them, instead of
only those that are "connected" via a destructor?
It looks like this base tracking logic is very old:
https://github.com/llvm/llvm-project/commit/9a220fca4a6f01f1d3363b3cb53f4f2fa77851c4
A few lines above we skip considering virtual bases, too, which seems relevant
to the problem at hand. Not ignoring a virtual base would probably avoid the
diagnostics, too.
https://github.com/llvm/llvm-project/blob/e87d34255382d8a8dc8dba13dbbeec3f0f634839/clang/lib/Sema/SemaCUDA.cpp#L501-L505
I'm reluctant to add a special case just for the destructor of an abstract
virtual base. It's a suspiciously specific exception for the code where we are
ignoring *kinds* of base classes.
For me this could be justified, provided that:
- we can prove that it's the only such corner case (maybe? I didn't manage to
find other ways to trigger the misdiagnostics so far), and
- including previously excluded abstract virtual base class has no unintended
consequences. This is hard to prove. We'll need to test it.
@yxsamliu WDYT?
https://github.com/llvm/llvm-project/pull/184894
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
