I found the terminator condition for binary operators a bit unintuitive in the CFG. I was wondering if I am missing something or it is a possible improvement.
Let’s consider the following simple code snippet:
void g(bool a, bool b) {
if (a || b) {
body();
} else {
body2();
}
}
The terminator condition for [B4] is (a cast above) a. This is pretty intuitive. This value will determine which branch do we take when we leave the basic block.
The terminator condition for [B3], however, is the whole binary or expression. I would expect it to be the right hand side of the logical or. What do you think? What is the reason that we have the whole expression here?
The terminator for [B3] is not the binary operator; it’s the whole if-statement. I guess it’s kind of generic if-statement logic here. It wouldn’t make that much sense to change the terminator here to ‘b’ if in all other places it’s still the if-statement. And i think we don’t really want to rewrite AST to create an ‘if (b)’ statement only to use it as a terminator, that’d be pretty weird.
But it might make sense if we change the behavior in all other places to avoid using the if-statement as a terminator. I’m not sure how much information would be lost in this case; it might work if we simply say in all places “here’s the condition expression, compute it, then jump to this or that block depending on the value” and simply never store control flow statements in the CFG. We might as well separate “terminator condition expression” from “terminator control flow statement”, not sure if i’m making any sense right now.
The logic in the Analyzer to act upon this part of CFG is pretty weird, cf. ExprEngine::VisitLogicalExpr(). It definitely needs a cleanup, probably with the help of a more easy-to-read CFG.
On second thought, it’s exactly how it works right now, if we declare that the “terminator condition expression” is just the last expression in the block. I.e., for [B4] the condition is ‘a’ and the terminator is ‘||’, for [B3] the condition is ‘b’ and the terminator is ‘if’.