[PATCH] Proposal on how to fix temporary dtors.

[Alex McCarthy]

Looking really good, thanks Manuel! I'll leave the final acceptance for Jordan or someone else with more clang cred than I.

It looks like this fixes all of the FIXMEs in temporaries.cpp. Does this (hopefully) close out all known bugs with temporary dtor handling?

If so, wooooo :D

================
Comment at: lib/StaticAnalyzer/Core/ExprEngine.cpp:1406
@@ -1390,7 +1405,3 @@
 
-  // FIXME: This is a workaround until we handle temporary destructor branches
-  // correctly; currently, temporary destructor branches lead to blocks that
-  // only have a terminator (and no statements). These blocks violate the
-  // invariant this function assumes.
-  if (B->getTerminator().isTemporaryDtorsBranch()) return Condition;
+  assert(!B->getTerminator().isTemporaryDtorsBranch());
 
----------------
Maybe add a message to this assert, like "Temporary destructor branches should be handled by processBindTemporary" or similar?

================
Comment at: lib/StaticAnalyzer/Core/ExprEngine.cpp:1435
@@ -1423,2 +1434,3 @@
                                const CFGBlock *DstF) {
+  assert(!Condition || !llvm::isa<CXXBindTemporaryExpr>(Condition));
   const LocationContext *LCtx = Pred->getLocationContext();
----------------
Consider adding an assert message

================
Comment at: lib/StaticAnalyzer/Core/ExprEngineCXX.cpp:25
@@ -24,1 +24,3 @@
 
+typedef std::pair<const CXXBindTemporaryExpr *, const StackFrameContext *>
+    CXXBindTemporaryContext;
----------------
Consider documenting that we need the stack frame to handle recursion

================
Comment at: lib/StaticAnalyzer/Core/ExprEngineCXX.cpp:190
@@ +189,3 @@
+  if (Pred->getState()->contains<InitializedTemporariesSet>(
+          std::make_pair(BTE, Pred->getStackFrame()))) {
+    TempDtorBuilder.markInfeasible(false);
----------------
is it worth adding a makeBindTemporaryContext(BTE, Pred) helper?

================
Comment at: test/Analysis/temporaries.cpp:255
@@ +254,3 @@
+
+  void testTernary(bool value) {
+    if (value) {
----------------
Can you add a ternary test that does generate expected warnings if the branching doesn't flow through a noreturn temporary dtor?

================
Comment at: test/Analysis/temporaries.cpp:281
@@ +280,3 @@
+
+  struct Dtor {
+    ~Dtor();
----------------
This looks identical to the namespaced struct defined on line 115: maybe extract that definition and remove this duplicate? (same with extern check)

================
Comment at: test/Analysis/temporaries.cpp:289
@@ +288,3 @@
+      // FIXME: Find a way to verify construction order.
+      // ~Dtor should run before ~NoReturnDtor() because construction order is
+      // guaranteed by comma operator.
----------------
Can we provide (inline) destructor bodies that do or don't warn to test this? (in a followup change)

http://reviews.llvm.org/D3627