NagyDonat wrote: > you foreshadowed more commits that you could see part of this PR. I didn't > want to immediately push back and reject the idea because again, that depend > on the nature of those changes.
Pushback against more commits on this already huge PR is very natural, I can readily accept it. I have only three suggestions that logically "belong to" this PR: - [A question about a newly added comment](https://github.com/llvm/llvm-project/pull/198211#discussion_r3305458865) that I don't understand. - [Completing the merging of implementations of `LocationContext` and `StackFrame`](https://github.com/llvm/llvm-project/pull/198211#discussion_r3305476828) - [Capitalizing a freshly introduced variable name](https://github.com/llvm/llvm-project/pull/198211#discussion_r3305653680) And there are two others that are very simple and clearly useful: - [Fixing an old typo on an already modified line](https://github.com/llvm/llvm-project/pull/198211#discussion_r3305658151) - [Deleting a factually inaccurate comment on an already modified line](https://github.com/llvm/llvm-project/pull/198211#discussion_r3305679709) The rest is just capitalizing unrelated variable names on lines that are touched by this commit. I'm personally annoyed by the lowercase variable names, and feel that they "look dirty", give a first impression that the code containing them was edited in a careless manner. This is a useful indicator in code that is actually old and hacky and awkwardly complicated, but I feel that a cleanup is "unfinished" if it leaves these harmless but ugly signs behind. However, I can understand it if this is my personal preference and I won't push for it against opposition. (I'd also be happy to create a separate PR that just renames variables on lines that were touched by this PR – I'm lobbying for including these changes in this PR primarily because I know that we have a policy against commits that just rename variables.) > The only way to differentiate from the changes we already have and the future > changes is to have them in separate commits, but then the next X commits > would blend into the 39 commits we already agreed on so far. I see, but it is easy to remember the number „39” and view the changes accordingly. In the Github UI it is easy to view the diff from many, but not all commits on the PR. https://github.com/llvm/llvm-project/pull/198211 _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
