[all-commits] [llvm/llvm-project] b343f3: [analyzer][NFC] Cleanup BranchNodeBuilder (#117898)

[Donát Nagy via All-commits]

  Branch: refs/heads/main
  Home:   https://github.com/llvm/llvm-project
  Commit: b343f3f3faf5b732cc19c9c51d392389677477ce
      https://github.com/llvm/llvm-project/commit/b343f3f3faf5b732cc19c9c51d392389677477ce
  Author: Donát Nagy <donat.nagy at ericsson.com>
  Date:   2024-12-02 (Mon, 02 Dec 2024)

  Changed paths:
    M clang/include/clang/StaticAnalyzer/Core/PathSensitive/CoreEngine.h
    M clang/lib/StaticAnalyzer/Core/CoreEngine.cpp
    M clang/lib/StaticAnalyzer/Core/ExprEngine.cpp

  Log Message:
  -----------
  [analyzer][NFC] Cleanup BranchNodeBuilder (#117898)

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.



To unsubscribe from these emails, change your notification settings at https://github.com/llvm/llvm-project/settings/notifications