[PATCH] D64464: [CodeGen] Emit destructor calls for non-trivial C structs

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.


================
Comment at: lib/CodeGen/CGExpr.cpp:4100
+  if (E->getType().isDestructedType() == QualType::DK_nontrivial_c_struct)
+    pushDestroy(QualType::DK_nontrivial_c_struct, DeclPtr, E->getType());
+
----------------
Unfortunately, the lifetime of compound literals in C is not this simple; they're like blocks in that they're destroyed at the end of the enclosing scope rather than at the end of the current statement. (The cleanup here will be popped at the end of the full-expression if we've entered an `ExprWithCleanups`.) And the l-value case is exactly the case where this matters.

I think you need to do something like what we do with blocks, where we record all the blocks in the full-expression on the `ExprWithCleanups` so that we can push an inactive cleanup for them and then activate it when we emit the block.


================
Comment at: lib/CodeGen/CGExprAgg.cpp:248
   bool RequiresDestruction =
-      Dest.isIgnored() &&
+      !Dest.isExternallyDestructed() &&
       RetTy.isDestructedType() == QualType::DK_nontrivial_c_struct;
----------------
ahatanak wrote:
> This change wasn't needed to fix the bugs, but I think `isExternallyDestructed` instead of `isIgnored` should be called to determine whether the returned value requires destruction.
Agreed.


================
Comment at: lib/CodeGen/CGExprAgg.cpp:668
+    Dest.setExternallyDestructed();
+  }
   CGF.EmitAggExpr(E->getInitializer(), Slot);
----------------
The cleanup shouldn't be pushed until after you've finished evaluating the initializer.


Repository:
  rC Clang

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

https://reviews.llvm.org/D64464