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

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added inline comments.


================
Comment at: clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp:1750
+  //                       [B1] -> [B2] -> [B3] -> [sink]
+  // assert(A && B || C);    \       \       \
+  //                          -------------------> [go on with the execution]
----------------
Szelethus wrote:
> NoQ wrote:
> > xazax.hun wrote:
> > > 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. 
> > *confirms that 3 successors is 1 successor too many*
> Alright. I think I got it now (eventually).
There shouldn't be a direct edge from `[B1]` to the sink. If `A` is true, we proceed to evaluate `B`. If `A` is false, we proceed to evaluate `C`.


================
Comment at: clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp:1753-1755
+  // It so happens that CFGBlock::getTerminatorCondition returns 'A' for block
+  // B1, 'A && B' for B2, and 'A && B || C' for B3. Let's check whether we
+  // reached the end of the condition!
----------------
Szelethus wrote:
> NoQ wrote:
> > Szelethus wrote:
> > > NoQ wrote:
> > > > Szelethus wrote:
> > > > > NoQ wrote:
> > > > > > Clever trick, but why not match for logical operators directly? Something like this:
> > > > > > ```lang=c++
> > > > > > if (auto B = dyn_cast<BinaryOperator>(OuterCond))
> > > > > >   if (B->isLogicalOp())
> > > > > >     return isAssertlikeBlock(Else, Context);
> > > > > > ```
> > > > > What about `assert(a + b && "Shouldn't be zero!");`?
> > > > Mmm, could you elaborate? >.<
> > > ```lang=c++
> > > if (auto B = dyn_cast<BinaryOperator>(OuterCond))
> > >   if (B->isLogicalOp())
> > >     return isAssertlikeBlock(Else, Context);
> > > ```
> > > We don't need `isLogicalOp()` here.
> > > 
> > > Also, I should probably have a testcase for the elvis operator (`?:`).
> > Mmm, i'm still confused. The `a + b` in your example doesn't appear as an else-block terminator anywhere. And i don't see why would `a + b` behave differently compared to a simple `a`, while i do see why would, say, `a && b` behave differently.
> Right. I take it all back. But I guess we might as well just `assert` that it's a logical operator, if it has 2 successors.
Aha, great!


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

https://reviews.llvm.org/D65287