[PATCH] D158156: [analyzer] Add C++ array delete checker

Mon Aug 21 10:47:36 PDT 2023

donat.nagy added a comment.

In D158156#4604242 <https://reviews.llvm.org/D158156#4604242>, @steakhal wrote:

> One more thing to mention. Its usually illadvised to rename files or do changes that would seriously impact git blame, unless we have a really good reason doing so.
> Aesthetics usually don't count one, especially if the name is not user-facing. However, maintainability is another axis, thus as it's always, not black and white.

Git (e.g. git blame) can be smart enough to recognize renamed files if there aren't too much changes, but this change may be too large for that (at least Phabricator doesn't recognize it as a move, which confused me at first when I looked at this review). I'd suggest keeping the old filename in this commit; if you wish (and the community accepts it) you could rename the file in a separate followup NFC commit that doesn't do anything else.

In D158156#4604221 <https://reviews.llvm.org/D158156#4604221>, @steakhal wrote:

> PS: sorry if any of my comments are dups, or outdated. I only had a little time last week, and things have progressed since then I noticed. I still decided to submit my possibly outdated and partial review comments. I hope you understand.
> Its quite difficult to allocate time to do reviews from day to to day work. I unfortunately usually do this in my spare time, if I have.

Thanks for reviewing our commits, it's very helpful for us.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D158156/new/

https://reviews.llvm.org/D158156