[PATCH] D65287: [analyzer][CFG] Don't track the condition of asserts
Artem Dergachev via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Tue Aug 13 17:13:10 PDT 2019
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]
> 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!
> 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.
CHANGES SINCE LAST ACTION
More information about the cfe-commits