[PATCH] D63538: [analyzer][CFG] Return the correct terminator condition

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

I think this kinda makes sense, but also it looks like a risky change, because who knows how many times we have took the old behavior for granted. In particular, i'm slightly worried about this breaking the wonky logic of `ExprEngine::VisitLogicalExpr` (cf. D59857 <https://reviews.llvm.org/D59857>). But i've no specific concerns, so i think it's worth a try, given that it's a massive simplification.



================
Comment at: clang/lib/Analysis/CFG.cpp:5628-5630
+  const Expr *Cond;
 
+  if (!(Cond = dyn_cast<Expr>(S))) {
----------------
Can we do the assignment to the previous line pls? ^.^


================
Comment at: clang/lib/Analysis/CFG.cpp:5631-5634
+    // Only ObjCForCollectionStmt is known not to be a non-Expr terminator.
+    const auto *O = cast<ObjCForCollectionStmt>(S);
 
+    Cond = O->getCollection();
----------------
The new arrow doesn't really make much sense. Like, there's nothing interesting going on specifically with the collection, so there's no reason to highlight it separately from, say, the element we're extracting from it. Let's just keep the old behavior.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D63538/new/

https://reviews.llvm.org/D63538