[PATCH] Proposal on how to fix temporary dtors.

[Manuel Klimek]

================
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());
 
----------------
Alex McCarthy wrote:
> Maybe add a message to this assert, like "Temporary destructor branches should be handled by processBindTemporary" or similar?
Done.

================
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();
----------------
Alex McCarthy wrote:
> Consider adding an assert message
Done.

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

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

================
Comment at: test/Analysis/temporaries.cpp:255
@@ +254,3 @@
+
+  void testTernary(bool value) {
+    if (value) {
----------------
Alex McCarthy wrote:
> Can you add a ternary test that does generate expected warnings if the branching doesn't flow through a noreturn temporary dtor?
Done. Aaaand it revealed a bug in the test. Nice catch!

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

================
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.
----------------
Alex McCarthy wrote:
> Can we provide (inline) destructor bodies that do or don't warn to test this? (in a followup change)
Not yet, for that we need to switch on inlining of destructors.

http://reviews.llvm.org/D3627