[analyzer][NFC] Cleanup BranchNodeBuilder #117898

NagyDonat · 2024-11-27T15:46:35Z

Previously BranchNodeBuilder had a machinery to mark the two possible branches (true, false) as infeasible, but this was completely useless in practice, because the BranchNodeBuilder objects where short-lived local variables so the markInfeasible() calls did not affect any later calls.

The only theoretical exception was that in ExprEngine::processBranch the methods of BranchNodeBuilder were called within a for loop that iterates over the nodes created by the check::BranchCondition callbacks.

However, currently only two checkers listen to check::BranchCondition and neither of them produces multiple out nodes. This is fortunate, because if the for loop had multiple iterations, then the lingering effects of markInfeasible() would have caused wildly incorrect behavior.

For example, let's assume that a hypothetical check::BranchCondition callback transitions to two different states, and the condition expression happens to be true in the first and false in the second. In this situation the first iteration of the loop would mark the false branch as 'infeasible' and then in the second iteration the analyzer would skip creating the transition to the false branch (from the state where that is the 'right' path forward).

After removing markInfeasible(), it became clear that the isFeasible() calls in ExprEngine::processBranch are redundant because they only guarded a generateNode call -- which immediately calls isFeasible() and does nothing on an infeasible branch.

At this point in the refactoring the only code writing the feasibility data members is their initialization in the constructors of BranchNodeBuilder and the only code reading them is the check at the beginning of BranchNodeBuilder::generateNode, so it was straightforward to remove them completely and simplify the logic of generateNode() to let it directly check the nullness of the CFGBlock* pointer that it wants to use.

Finally, after these changes it became clear that in ExprEngine::processBranch the else branch
(corresponding to the case when assumeCondition fails) is equivalent to the "normal" case, so I eliminated it as well.

I also update the capitalization of a few variables that are already affected by this change.

Previously `BranchNodeBuilder` had a machinery to mark the two possible branches (true, false) as infeasible, but this was completely useless in practice, because the `BranchNodeBuilder` objects where short-lived local variables so the `markInfeasible()` calls did not affect any later calls. The only theoretical exception was that in `ExprEngine::processBranch` the methods of `BranchNodeBuilder` were called within a `for` loop that iterates over the nodes created by the `check::BranchCondition` callbacks. However, currently only two checkers listen to `check::BranchCondition` and neither of them produces multiple out nodes. This is fortunate, because if the `for` loop had multiple iterations, then the lingering effects of `markInfeasible()` would have caused wildly incorrect behavior. _For example, let's assume that a hypothetical `check::BranchCondition` callback transitions to two different states, and the condition expression happens to be true in the first and false in the second. In this situation the first iteration of the loop would mark the false branch as 'infeasible' and then in the second iteration the analyzer would skip creating the transition to the false branch (from the state where that is the 'right' path forward)._ After removing `markInfeasible()`, it became clear that the `isFeasible()` calls in `ExprEngine::processBranch` are redundant because they only guarded a `generateNode` call -- which immediately calls `isFeasible()` and does nothing on an infeasible branch. At this point in the refactoring the only code writing the feasibility data members is their initialization in the constructors of `BranchNodeBuilder` and the only code reading them is the check at the beginning of `BranchNodeBuilder::generateNode`, so it was straightforward to remove them completely and simplify the logic of `generateNode()` to let it directly check the nullness of the `CFGBlock*` pointer that it wants to use. Finally, after these changes it became clear that in `ExprEngine::processBranch` the `else` branch (corresponding to the case when `assumeCondition` fails) is equivalent to the "normal" case, so I eliminated it as well.

llvmbot · 2024-11-27T15:47:10Z

@llvm/pr-subscribers-clang-static-analyzer-1

@llvm/pr-subscribers-clang

Author: Donát Nagy (NagyDonat)

Changes