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