[PATCH] D65287: [analyzer][CFG] Don't track the condition of asserts

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun added inline comments.


================
Comment at: clang/include/clang/Analysis/CFG.h:859
+  /// Returns true if the block would eventually end with a sink (a noreturn
+  /// node). We scan the child CFG blocks in a depth-first manner and see if
+  /// all paths eventually end up in an immediate sink block.
----------------
I think the depth-first manner is an implementation detail and the result of this method should not depend on the traversal strategy. I would remove that part from the comment.


================
Comment at: clang/lib/Analysis/CFG.cpp:5634
+  // immediately caught.
+  if (std::any_of(Blk->begin(), Blk->end(), [](const CFGElement &Elm) {
+        if (Optional<CFGStmt> StmtElm = Elm.getAs<CFGStmt>())
----------------
I am wondering, should we actually scan the whole `CFGBlock` for `ThrowExpr`s? Shouldn't `Throw` be the terminator? (In case we can get that easily) In case we are afraid of seeing lifetime end or other blocks AFTER throw, maybe it is more efficient to start scanning from the last element ad early return on the first non-throw stmt?


================
Comment at: clang/lib/Analysis/CFG.cpp:5649
+  const CFGBlock *StartBlk = this;
+  if (!StartBlk)
+    return false;
----------------
This should never be null, I think this should be dead code.


================
Comment at: clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp:1750
+  //                       [B1] -> [B2] -> [B3] -> [sink]
+  // assert(A && B || C);    \       \       \
+  //                          -------------------> [go on with the execution]
----------------
baloghadamsoftware wrote:
> xazax.hun wrote:
> > I wonder if the CFG is correct for your example. If A evaluates to false, we still check C before the sink. If A evaluates to true we still check B before going on. But maybe it is just late for me :)
> I think an arrow from `[B1]` to `[B3]` is missing for the `A == false` case.
I am still not sure I like this picture. Looking at [B1] I had the impression it has 3 successors which is clearly not the case. 


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

https://reviews.llvm.org/D65287