[clang] [llvm] [NFC][analyzer] Extract bounds checking library (PR #202372)

[Donát Nagy via cfe-commits]

NagyDonat wrote:

> This PR is not just moving code between files, many of the facilities it introduces like CheckResult did not exist before.

Before this PR, the generalizable logic and the concrete features of `security.ArrayBound` were tightly intertwined, so I had to do some refactoring (e.g. introducing `CheckResult`) to be able to move the code that I want to move.

> It might make sense to do the move and the redesign in two steps but in this case I'd prefer to start with a PR that purely just moves code.

It was impossible to start with a "pure move", but in hindsight it might have been better to start with a "pure redesign" commit that leaves everything in `ArrayBoundChecker.cpp` but introduces `CheckResult`, renames methods etc. (and after that we could've had a pure move commit).

However, these paths both lead to the same end result and the only drawback of the current approach is that for a few weeks (until we merge my next PR) the code will be uglier.

> Currently, there are some arbitrary changes you made to the code and some you rejected 

Many of the changes that I did were necessary to separate the moved logic from the logic that stays in the checker file. (E.g. previously `ArrayBoundChecker::performCheck` did the bounds check _and_ emitted the bug report; now `bounds::checkBounds` does the bounds check and returns a `CheckResult`, while `ArrayBoundChecker::handleAccessExpr` inspects this result and emits the bug report.)

Some of my changes were not strictly necessary for the move (e.g. renaming `getMessage` to `getAssumptionMsg` just because the old name was terribly vague), I probably should revert these.

> and there seems to be no clear distinction what changes are OK and what are not and why.

In my opinion the primary problem is when a function is _moved and renamed, but otherwise mostly unchanged_, because in this case we lose track of its history.
- If the code is not moved, we have reasonable `git blame` even if the name changes.
- If the name is preserved, it is straightforward to find the same logic in the revision before the move and follow its history there.
- If the code is new or heavily modified, then there is no old history that we need to find.

Also, this is not a clear black-and-white distinction, but a "try to reduce the amount of disruption" situation. Renaming a few functions wouldn't be a fatal issue, but I don't see a reason to do it in this commit instead of postponing it to a follow-up PR.

In this PR I intend to polish the freshly introduced interfaces (the surfaces where the old monolith was "broken in half") – but I would like to leave cleanup of existing code and naming choices to a follow-up PR.



https://github.com/llvm/llvm-project/pull/202372