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