[PATCH] D77081: [MS] Mark vbase dtors ref'd when ref'ing dtor

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith marked an inline comment as done.
rsmith added inline comments.


================
Comment at: clang/lib/Sema/SemaExpr.cpp:16008-16013
+            // In the MS ABI, the complete destructor is implicitly defined,
+            // even if the base destructor is user defined.
+            CXXRecordDecl *Parent = Destructor->getParent();
+            if (Parent->getNumVBases() > 0 &&
+                !Parent->definedImplicitCompleteDestructor())
+              DefineImplicitCompleteDestructor(Loc, Destructor);
----------------
rnk wrote:
> rsmith wrote:
> > Can we avoid doing this when we know the call is to a base subobject destructor or uses virtual dispatch? Should we? (I mean I guess ideally we'd change this function to take a `GlobalDecl` instead of a `FunctionDecl*`, but that seems like a lot of work.)
> > 
> > How does MSVC behave?
> I think we can skip this if virtual dispatch is used, but I'm not sure what kinds of devirtualization might break my assumptions.
> 
> For example, uncommenting `final` here causes MSVC to diagnose the error, but it otherwise it compiles:
> ```
> struct NoDtor { ~NoDtor() = delete; };
> struct DefaultedDtor : NoDtor { ~DefaultedDtor() = default; };
> struct HasVBases /*final*/ : virtual DefaultedDtor {
>   virtual ~HasVBases();
> };
> void deleteIt1(HasVBases *p) { delete p; }
> ```
> 
> If the NoDtor destructor is undeleted and the class is made final, then both deleting and complete dtors are emitted for HasVBases. Clang would need to mark DefaultedDtor referenced in this case.
> 
> I think it's OK to "overdiagnose" in this case. It's not possible to emit a vtable here without diagnosing the error.
Great, if we're happy to treat all uses of the destructor as using the complete object destructor, then doing this work as part of marking the destructor used seems like it would be the best solution.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D77081/new/

https://reviews.llvm.org/D77081