[PATCH] Proposal on how to fix temporary dtors.

[Alex McCarthy]

This is awesome, thanks Manuel! I can't wait to see this working :)

================
Comment at: lib/Analysis/CFG.cpp:3540
@@ -3582,60 +3539,3 @@
     AbstractConditionalOperator *E, bool BindToTemporary) {
-  // First add destructors for condition expression.  Even if this will
-  // unnecessarily create a block, this block will be used at least by the full
-  // expression.
-  autoCreateBlock();
-  CFGBlock *ConfluenceBlock = VisitForTemporaryDtors(E->getCond());
-  if (badCFG)
-    return NULL;
-  if (BinaryConditionalOperator *BCO
-        = dyn_cast<BinaryConditionalOperator>(E)) {
-    ConfluenceBlock = VisitForTemporaryDtors(BCO->getCommon());
-    if (badCFG)
-      return NULL;
-  }
-
-  // Try to add block with destructors for LHS expression.
-  CFGBlock *LHSBlock = NULL;
-  Succ = ConfluenceBlock;
-  Block = NULL;
-  LHSBlock = VisitForTemporaryDtors(E->getTrueExpr(), BindToTemporary);
-  if (badCFG)
-    return NULL;
-
-  // Try to add block with destructors for RHS expression;
-  Succ = ConfluenceBlock;
-  Block = NULL;
-  CFGBlock *RHSBlock = VisitForTemporaryDtors(E->getFalseExpr(),
-                                              BindToTemporary);
-  if (badCFG)
-    return NULL;
-
-  if (!RHSBlock && !LHSBlock) {
-    // If neither LHS nor RHS expression had temporaries to destroy don't create
-    // more blocks.
-    Block = ConfluenceBlock;
-    return Block;
-  }
-
-  Block = createBlock(false);
-  Block->setTerminator(CFGTerminator(E, true));
-  assert(Block->getTerminator().isTemporaryDtorsBranch());
-
-  // See if this is a known constant.
-  const TryResult &KnownVal = tryEvaluateBool(E->getCond());
-
-  if (LHSBlock) {
-    addSuccessor(Block, LHSBlock, !KnownVal.isFalse());
-  } else if (KnownVal.isFalse()) {
-    addSuccessor(Block, NULL);
-  } else {
-    addSuccessor(Block, ConfluenceBlock);
-    std::reverse(ConfluenceBlock->pred_begin(), ConfluenceBlock->pred_end());
-  }
-
-  if (!RHSBlock)
-    RHSBlock = ConfluenceBlock;
-
-  addSuccessor(Block, RHSBlock, !KnownVal.isTrue());
-
-  return Block;
+  CFGBlock *CondBlock = VisitForTemporaryDtors(E->getCond());
+  CFGBlock *TrueBlock = VisitForTemporaryDtors(E->getTrueExpr());
----------------
Noob question: If we return FalseBlock below, do we need to visit CondBlock and TrueBlock even if they aren't returned? If we do need to visit them, is the order important? (Consider documenting this if this isn't super-obvious to someone with more clang experience than me :) )

================
Comment at: lib/StaticAnalyzer/Core/ExprEngine.cpp:771
@@ +770,3 @@
+             !State->contains<InitializedTemporariesSet>(BTE));
+      State = State->add<InitializedTemporariesSet>(BTE);
+      StmtBldr.generateNode(BTE, Pred, State);
----------------
If includeTemporaryDtorsInCFG is not set, should we avoid this state modification?

================
Comment at: lib/StaticAnalyzer/Core/ExprEngine.cpp:1451
@@ -1433,1 +1450,3 @@
 
+  if (const CXXBindTemporaryExpr *BTE = 
+      dyn_cast<CXXBindTemporaryExpr>(Condition)) {
----------------
I think it's worth pulling this out into a new processTemporary helper method rather than adding it to processBranch: you're not reusing any of the logic in processBranch in the temporary dtor case.

This would involve adding a new HandleTemporary helper (or similar) that you'd call instead of HandleBranch, and that would call processTemporary

================
Comment at: test/Analysis/temporaries.cpp:269
@@ +268,3 @@
+      if (i < 3 && (i >= 2 || check(NoReturnDtor()))) {
+        // FIXME: Figure out why we don't run into this for i == 2.
+        // If we add if (i == 2 && (...)) we run into this case, though.
----------------
Don't we hit the NoReturnDtor when i == 0, which will prevent us from getting to i==2? This seems correct as-is.

================
Comment at: test/Analysis/temporaries.cpp:275
@@ -228,2 +274,3 @@
+  }
 
   void testBinaryOperatorShortcut(bool value) {
----------------
Can you add a test that cleans up multiple temporaries in one condition? Maybe something like this (not sure if this compiles, we might need to add a check2(obj1, obj2) function):
  class Dtor {
    ~Dtor() {}
  };

  void testMultipleTemporaries(bool value) {
    if (value) {
      // ~Dtor should run before ~NoReturnDtor() because construction order is guaranteed by comma operator
      if (!value || check((NoReturnDtor(), Dtor())) || value) {
        clang_analyzer_eval(true); // no warning, unreachable code
      }
    }
  }

We could also make ~Dtor emit a warning which would only be triggered if Dtor was constructed with a certain value (e.g. check(Dtor(/*warning=*/true), NoReturnDtor()), then see if clang warns on that error that should never happen.

http://reviews.llvm.org/D3627