[PATCH] D64569: [OpenCL] Improve destructor support in C++ for OpenCL

[Marco Antognini via Phabricator via cfe-commits]

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