<div dir="ltr">I'd add that cleanups can create conflicts with pending patches, which isn't major, but raises the bar for global cleanups slightly.<div><br></div><div>Cleaning up code before modifying it is totally reasonable because you're already going to change it. In effect you're taking some ownership for it. If problems arise as a result of your cleanups or functional changes, you're on the hook to fix it. It also usually reduces the patch size of the eventual functional change, which makes it easier to understand the patch during code review and in the future during source code archaeology.</div><div><br></div><div>I think we should basically do what you suggest: if people want to make global pattern-based cleanups (push_back -> emplace_back, range-based-for, etc), we should just raise it on llvm-dev and make a decision about the value of the cleanup.</div></div><div class="gmail_extra"><br><div class="gmail_quote">On Fri, Apr 7, 2017 at 3:05 PM, David Blaikie via llvm-dev <span dir="ltr"><<a href="mailto:llvm-dev@lists.llvm.org" target="_blank">llvm-dev@lists.llvm.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">There have been some efforts recently to use clang-tidy or similar automated refactoring to make project-wide cleanups/improvements to the LLVM codebase that appear to me to be a bit out of character with the sort of changes & philosophies that have been applied in the past.<br><br>I think there are a few issues at hand which I'll try to summarize:<br><br>* Churn/blame-convenience:<br>Previously, large scale changes (classically whitespace cleanup, with things like clang-format) have been rejected or discouraged due to the risk of making it harder to find the original source of a semantic change.<br>I'm not too wedded to this issue myself, but it did seem to be the status-quo previously so I'm curious to better understand if/how people see this applying or not, to more semantic changes.<br><br>* Efficiency:<br>Sending individual or even batched reviews for automated changes like this seems inefficient. I'd rather see, for example, an email to llvm-dev to discuss whether a particular change makes sense to make across the project (ie: does the LLVM project want to cleanup all instances of classic for to range-based-for when clang-tidy can do so?) and then not send the individual changes for review, but commit them directly where possible. (if the person doing this automated cleanup doesn't have permission, yeah, send it out and reference the original discussion thread).<br><br>Some points of comparison: sometimes there's similar cleanup done in a non-automated fashion as Mehdi pointed out in one of the threads I brought this up (see this thread: <a href="http://lists.llvm.org/pipermail/llvm-commits/Week-of-Mon-20170403/443547.html" target="_blank">http://lists.llvm.org/<wbr>pipermail/llvm-commits/Week-<wbr>of-Mon-20170403/443547.html</a> ). Usually some amount of cleanup has been acceptable if the code was generally being churned anyway (eg: clang-formatting a file so it's consistent, before doing major surgery to it anyway, so the surgical changes don't create formatting inconsistencies), or as a result of a new API change (add a range-based accessor then fix up existing call sites to use range-based-for). I'd also not be surprised by a whitespace cleanup shortly after new code was committed - and have done this myself "oops, forgot to format my commit, here's a new commit that does the formatting". That seems different to me from these wider-scale cleanups.<br><br>I think I'm personally mostly in favor of this sort of stuff (though I think I would like some community buy-in to sign off project-wide on each clang-tidy rule/pattern/etc that's going to be applied) but it does seem new/different from the way some things have been done in the past so I'm curious how other people think about the differences/similarities/<wbr>guiding principles.<span class="HOEnZb"><font color="#888888"><br><br>- David</font></span></div>
<br>______________________________<wbr>_________________<br>
LLVM Developers mailing list<br>
<a href="mailto:llvm-dev@lists.llvm.org">llvm-dev@lists.llvm.org</a><br>
<a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/<wbr>mailman/listinfo/llvm-dev</a><br>
<br></blockquote></div><br></div>