[PATCH] Fix crash when resolving branch conditions for temporary destructor condition blocks.

[Jordan Rose]

What this is basically saying is that we will never be in a situation where the condition (or the rightmost branch in the condition) was not evaluated in the current CFG block. That seems a bit questionable in general but reasonable given the way the CFG is currently constructed. (Especially if/when this whole block of code can go away!)

I'm concerned about the test change Alex noted but otherwise this looks good.

================
Comment at: lib/StaticAnalyzer/Core/ExprEngine.cpp:1355
@@ -1354,1 +1354,3 @@
 
+const Stmt* GetRightmostLeaf(const Stmt* Condition) {
+  while (Condition) {
----------------
Please follow the new LLVM style with lowercase function names. This can also be static. (And the asterisks go on the right of the space.)

================
Comment at: lib/StaticAnalyzer/Core/ExprEngine.cpp:1389
@@ -1363,1 +1388,3 @@
 
+  // FIXME: This is a workaround until we handle temporary destructor branches
+  // correctly; currently, temporary destructor branches lead to blocks that
----------------
Alex McCarthy wrote:
> Consider linking to http://lists.cs.uiuc.edu/pipermail/cfe-dev/2014-April/036653.html for context
No, we try not to put links or PR numbers in the source code itself. Commit messages, sure.

================
Comment at: lib/StaticAnalyzer/Core/ExprEngine.cpp:1410
@@ -1377,13 +1409,3 @@
       continue;
-    if (CS->getStmt() != Condition)
-      break;
-    return Condition;
-  }
-
-  assert(I != E);
-
-  while (Condition) {
-    BO = dyn_cast<BinaryOperator>(Condition);
-    if (!BO || !BO->isLogicalOp())
-      return Condition;
-    Condition = BO->getRHS()->IgnoreParens();
+    const clang::Stmt* LastStmt = CS->getStmt();
+    assert(LastStmt == Condition || LastStmt == GetRightmostLeaf(Condition));
----------------
Style nitpicks: no "clang::", asterisk next to the variable name.

================
Comment at: lib/StaticAnalyzer/Core/ExprEngine.cpp:1414
@@ -1390,3 +1413,3 @@
   }
   llvm_unreachable("could not resolve condition");
 }
----------------
Alex McCarthy wrote:
> What happens if we actually hit an llvm_unreachable? Does it die like an assert?
> 
> If not, maybe assert before this?
It's like an assert when assertions are enabled, but acts as an optimization hint in NDEBUG builds. So it's slightly better than an assert.

================
Comment at: test/Analysis/temporaries.cpp:193
@@ -194,3 +192,3 @@
 
-    FIXME: This shouldn't cause a warning.
-    if (compute(i == 5 &&
+    if (check(NoReturnDtor()) &&
+        compute(i == 5 &&
----------------
Alex McCarthy wrote:
> Out of curiosity, why the change to this test case?
Seconded; all the existing tests are designed to tickle past bugs, and I'd rather not change any of the conditions.

http://reviews.llvm.org/D3552