[llvm-branch-commits] [clang] [clang] Use uniform lifetime bounds under exceptions (PR #175817)

[Paul Kirth via llvm-branch-commits]

================
@@ -6686,8 +6686,22 @@ Expr *Sema::MaybeCreateExprWithCleanups(Expr *SubExpr) {
   assert(ExprCleanupObjects.size() >= FirstCleanup);
   assert(Cleanup.exprNeedsCleanups() ||
          ExprCleanupObjects.size() == FirstCleanup);
-  if (!Cleanup.exprNeedsCleanups())
+  if (!Cleanup.exprNeedsCleanups()) {
+    // If we have a 'new' expression with a non-trivial destructor, we need to
+    // wrap it in an ExprWithCleanups to ensure that the destructor is called
+    // if the constructor throws.
+    if (auto *NE = dyn_cast<CXXNewExpr>(SubExpr)) {
----------------
ilovepi wrote:

I mostly got that to work. It broke  a test under c++98 (cwg3xx.cpp). I stepped through in a debugger, and near as I can tell, that test broke because the static assert din't recognize ExprWithCleanups as transparent to evaluation in checkICE.  I'm not super confident that is 100% the right course of action, but I kind of convinced myself it should be OK skimming the other special cases and AST documentation. Happy to try another approach if I'm wrong, but all the existing tests pass with this, so if we're not happy I guess we (I) should try and figure out a counter example to add as a regression test.

https://github.com/llvm/llvm-project/pull/175817