rjmccall added inline comments.
================
Comment at: clang/lib/CodeGen/CGClass.cpp:2016
CGF.EmitCXXDestructorCall(dtor, Dtor_Complete, /*for vbase*/ false,
- /*Delegating=*/false, addr);
+ /*Delegating=*/false, addr, type);
}
----------------
mantognini wrote:
> rjmccall wrote:
> > mantognini wrote:
> > > This is the only place where the new parameter has an interesting value.
> > > In the ~10 other calls to `EmitCXXDestructorCall` overloads, the default
> > > value of this new parameter is used instead.
> > Arguments that are potentially required for correctness — as opposed to
> > just enabling some optimization — should generally not have defaults. I
> > think you should remove these defaults and require a sensible type to be
> > passed down in all cases.
> I've addressed that. I had to add some extra parameter/attributes here and
> there. Please let me know if anything is can be improved.
Thanks, other than the FIXMEs and the object/pointer mismatches this all looks
right; very nicely done.
================
Comment at: clang/lib/CodeGen/CGClass.cpp:496
+ // destroyed should have the expected type.
+ QualType ThisTy = D->getThisType();
Address Addr =
----------------
I think the rule we want is that the type passed here is the (qualified) object
type, but `getThisType()` will return a pointer type. Consider adding a
`getThisObjectType()` method to `CXXMethodDecl` which does that computation
(and then make `getThisType()` just wrap that in a `PointerType`).
================
Comment at: clang/lib/CodeGen/CGClass.cpp:1447
+ if (HaveInsertPoint()) {
+ // FIXME: Determine the type of the object being destroyed.
+ QualType ThisTy;
----------------
mantognini wrote:
> I'm not familiar enough with CXXCtorType and such, and would welcome some
> help for these two FIXMEs.
`Dtor->getThisObjectType()` should be right in both of these cases.
================
Comment at: clang/lib/CodeGen/CGClass.cpp:2376
+ // Therefore, "this" should have the expected type.
+ QualType ThisTy = Dtor->getThisType();
CGF.EmitCXXDestructorCall(Dtor, Type, /*ForVirtualBase=*/false,
----------------
Same thing about `getThisObjectType()`.
================
Comment at: clang/lib/CodeGen/CGExprCXX.cpp:103
+ // we ensure a cast is added where necessary.
+ if (ThisTy.isNull()) {
+#ifndef NDEBUG
----------------
mantognini wrote:
> Despite no longer having a default parameter, not all call site can provide a
> meaningful value ATM. That is why this check is still required.
Is that resolved with fixing the FIXME?
Please assert that `ThisTy->getAsCXXRecordDecl() == Dtor->getParent()` to guard
against that pointer/object mixup.
================
Comment at: clang/lib/CodeGen/CGExprCXX.cpp:374
EmitCXXDestructorCall(GD, Callee, This.getPointer(),
+ /*ThisTy=*/QualType(),
/*ImplicitParam=*/nullptr,
----------------
`ThisTy` here can be either `Base->getType()` or
`Base->getType()->getPointeeType()` depending on whether this is an arrow
access.
================
Comment at: clang/lib/CodeGen/ItaniumCXXABI.cpp:1758
+ // This is suboptimal for correctness purposes. Instead, CE should probably
+ // always be defined.
+ QualType ThisTy;
----------------
It looks like the only places that pass down a null CE are the two
implementations in `emitVirtualObjectDelete`, which both have a
`CXXDeleteExpr`. You could just have this method take an
`llvm::PointerUnion<CXXMethodCallExpr*,CXXDeleteExpr*>` or something, and then
someone else could take advantage of that for better source locations on the
virtual call eventually.
================
Comment at: clang/lib/CodeGen/MicrosoftCXXABI.cpp:1928
+ if (CE)
+ ThisTy = CE->getImplicitObjectArgument()->getType();
+
----------------
Same point as in the ItaniumCXXABI implementation.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D64569/new/
https://reviews.llvm.org/D64569
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
