mantognini marked an inline comment as done.
mantognini added inline comments.
================
Comment at: clang/lib/CodeGen/CGClass.cpp:496
+ // destroyed should have the expected type.
+ QualType ThisTy = D->getThisType();
Address Addr =
----------------
rjmccall wrote:
> 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`).
I've done the suggested refactoring, let me know if it needs adjustment.
================
Comment at: clang/lib/CodeGen/CGExprCXX.cpp:103
+ // we ensure a cast is added where necessary.
+ if (ThisTy.isNull()) {
+#ifndef NDEBUG
----------------
rjmccall wrote:
> 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.
Yes, indeed. I've therefore simplified the code and added the requested
assertion.
================
Comment at: clang/lib/CodeGen/CGExprCXX.cpp:374
EmitCXXDestructorCall(GD, Callee, This.getPointer(),
+ /*ThisTy=*/QualType(),
/*ImplicitParam=*/nullptr,
----------------
rjmccall wrote:
> `ThisTy` here can be either `Base->getType()` or
> `Base->getType()->getPointeeType()` depending on whether this is an arrow
> access.
Thanks for mentioning it. It should be better now.
================
Comment at: clang/lib/CodeGen/CGExprCXX.cpp:99
+ assert(!ThisTy.isNull());
+ assert(!ThisTy->isPointerType() && "Unexpected pointer type");
+ assert(ThisTy->getAsCXXRecordDecl() == DtorDecl->getParent() &&
----------------
I wasn't 100% sure if this was covered by the next assertion.
================
Comment at: clang/lib/CodeGen/ItaniumCXXABI.cpp:1758
+ // This is suboptimal for correctness purposes. Instead, CE should probably
+ // always be defined.
+ QualType ThisTy;
----------------
rjmccall wrote:
> 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.
Alright, makes sense. I've changed the function to take a `PointerUnion` and
removed the FIXMEs.
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
