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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
