From 076b120bebfd727b502208601012a44ab2e1028e Mon Sep 17 00:00:00 2001
From: Richard Smith <richard@metafoo.co.uk>
Date: Wed, 5 Aug 2020 14:52:24 -0700
Subject: [PATCH] CFG: Destroy temporaries in (a,b) expression in the correct
 order.

---
 clang/lib/Analysis/CFG.cpp  | 20 ++++++++------------
 clang/test/Analysis/cfg.cpp | 18 ++++++++++++++++++
 2 files changed, 26 insertions(+), 12 deletions(-)

diff --git a/clang/lib/Analysis/CFG.cpp b/clang/lib/Analysis/CFG.cpp
index fc74226..cde0bf4 100644
--- a/clang/lib/Analysis/CFG.cpp
+++ b/clang/lib/Analysis/CFG.cpp
@@ -4773,11 +4773,11 @@ CFGBlock *CFGBuilder::VisitChildrenForTemporaryDtors(Stmt *E,
 CFGBlock *CFGBuilder::VisitBinaryOperatorForTemporaryDtors(
     BinaryOperator *E, bool ExternallyDestructed, TempDtorContext &Context) {
   if (E->isCommaOp()) {
-    // For comma operator LHS expression is visited
-    // before RHS expression. For destructors visit them in reverse order.
-    CFGBlock *RHSBlock = VisitForTemporaryDtors(E->getRHS(), ExternallyDestructed, Context);
+    // For the comma operator, the LHS expression is evaluated before the RHS
+    // expression, so prepend temporary destructors for the LHS first.
     CFGBlock *LHSBlock = VisitForTemporaryDtors(E->getLHS(), false, Context);
-    return LHSBlock ? LHSBlock : RHSBlock;
+    CFGBlock *RHSBlock = VisitForTemporaryDtors(E->getRHS(), ExternallyDestructed, Context);
+    return RHSBlock ? RHSBlock : LHSBlock;
   }
 
   if (E->isLogicalOp()) {
@@ -4798,19 +4798,15 @@ CFGBlock *CFGBuilder::VisitBinaryOperatorForTemporaryDtors(
   }
 
   if (E->isAssignmentOp()) {
-    // For assignment operator (=) LHS expression is visited
-    // before RHS expression. For destructors visit them in reverse order.
+    // For assignment operators, the RHS expression is evaluated before the LHS
+    // expression, so prepend temporary destructors for the RHS first.
     CFGBlock *RHSBlock = VisitForTemporaryDtors(E->getRHS(), false, Context);
     CFGBlock *LHSBlock = VisitForTemporaryDtors(E->getLHS(), false, Context);
     return LHSBlock ? LHSBlock : RHSBlock;
   }
 
-  // For any other binary operator RHS expression is visited before
-  // LHS expression (order of children). For destructors visit them in reverse
-  // order.
-  CFGBlock *LHSBlock = VisitForTemporaryDtors(E->getLHS(), false, Context);
-  CFGBlock *RHSBlock = VisitForTemporaryDtors(E->getRHS(), false, Context);
-  return RHSBlock ? RHSBlock : LHSBlock;
+  // Any other operator is visited normally.
+  return VisitChildrenForTemporaryDtors(E, ExternallyDestructed, Context);
 }
 
 CFGBlock *CFGBuilder::VisitCXXBindTemporaryExprForTemporaryDtors(
diff --git a/clang/test/Analysis/cfg.cpp b/clang/test/Analysis/cfg.cpp
index 0159a3c..5f2b365 100644
--- a/clang/test/Analysis/cfg.cpp
+++ b/clang/test/Analysis/cfg.cpp
@@ -575,6 +575,24 @@ int vla_evaluate(int x) {
   return x;
 }
 
+// CHECK-LABEL: void CommaTemp::f()
+// CHECK:       [B1]
+// CHECK-NEXT:    1: CommaTemp::A() (CXXConstructExpr,
+// CHECK-NEXT:    2: [B1.1] (BindTemporary)
+// CHECK-NEXT:    3: CommaTemp::B() (CXXConstructExpr,
+// CHECK-NEXT:    4: [B1.3] (BindTemporary)
+// CHECK-NEXT:    5: ... , [B1.4]
+// CHECK-NEXT:    6: ~CommaTemp::B() (Temporary object destructor)
+// CHECK-NEXT:    7: ~CommaTemp::A() (Temporary object destructor)
+namespace CommaTemp {
+  struct A { ~A(); };
+  struct B { ~B(); };
+  void f();
+}
+void CommaTemp::f() {
+  A(), B();
+}
+
 // CHECK-LABEL: template<> int *PR18472<int>()
 // CHECK: [B2 (ENTRY)]
 // CHECK-NEXT:   Succs (1): B1
-- 
2.7.4