
<html><head><meta http-equiv="Content-Type" content="text/html charset=utf-8"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;" class=""><br class=""><div><blockquote type="cite" class=""><div class="">On Apr 7, 2017, at 11:30 AM, David Blaikie <<a href="mailto:dblaikie@gmail.com" class="">dblaikie@gmail.com</a>> wrote:</div><br class="Apple-interchange-newline"><div class=""><div dir="ltr" class=""><br class=""><br class=""><div class="gmail_quote"><div dir="ltr" class="">On Fri, Apr 7, 2017 at 11:18 AM Mehdi Amini <<a href="mailto:mehdi.amini@apple.com" class="">mehdi.amini@apple.com</a>> wrote:<br class=""></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word" class="gmail_msg"><div class="gmail_msg"><blockquote type="cite" class="gmail_msg"><div class="gmail_msg">On Mar 6, 2017, at 2:36 PM, David Blaikie via llvm-commits <<a href="mailto:llvm-commits@lists.llvm.org" class="gmail_msg" target="_blank">llvm-commits@lists.llvm.org</a>> wrote:</div><br class="gmail_msg m_-8683649674969322270Apple-interchange-newline"><div class="gmail_msg"><div dir="ltr" class="gmail_msg">In the past the LLVM Project's generally avoided mass cleanup (even whitespace - which is a bit easier to workaround) due to the issues it has with version history and churn.<br class="gmail_msg"></div></div></blockquote><div class="gmail_msg"><br class="gmail_msg"></div></div></div><div style="word-wrap:break-word" class="gmail_msg"><div class="gmail_msg"><div class="gmail_msg">I don’t agree with this statement: I don’t believe we avoided *cleanup* but instead we avoided *mass formatting* changes.</div></div></div></blockquote><div class=""><br class="">What's your take on the motivation for that avoidance? For me it seemed like the avoidance was that the benefit wasn't worth the churn/blame/etc.<br class=""></div></div></div></div></blockquote><div><br class=""></div><div>What is “churn” and why is it “bad"?</div><div><br class=""></div><div>The only thing is "git blame”, which is a very minor avoidance IMO (this is a tooling issue and is easily worked around). Massive Formatting are usually touching *most* of the file, and the benefit is sometimes minimal (single whitespace change?) compare to the spread of the changes, which makes it “controversial” (I’d still do it, just like LLDB did recently).</div><div><br class=""></div><div>The changes here are touching a small amount of lines per file and every line changed is a “semantic” change (include order, etc.), that seems different enough to “formatting”.</div><div><br class=""></div><blockquote type="cite" class=""><div class=""><div dir="ltr" class=""><div class="gmail_quote"><div class=""><br class="">To me that motivation potentially applies to more than just whitespace & some of these changes seem like they maybe fall under a similar categorization.</div><div class=""> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word" class="gmail_msg"><div class="gmail_msg"><br class="gmail_msg"><blockquote type="cite" class="gmail_msg"><div class="gmail_msg"><div dir="ltr" class="gmail_msg">I believe I've mentioned/suggested this in the past - but this sort of cleanup is probably worth an llvm-dev discussion to figure out what the right path is for it in general. (in that conversation I'd point out that without tool integration to /maintain/ these invariants/stylistic rules, it's off less/little value to do the cleanup as it'll regress relatively quickly)<br class="gmail_msg"><br class="gmail_msg">Please start an llvm-dev thread and get some kind of closure on the best strategy before continuing with these sort of changes.</div></div></blockquote><div class="gmail_msg"><br class="gmail_msg"></div></div></div><div style="word-wrap:break-word" class="gmail_msg"><div class="gmail_msg"><div class="gmail_msg">We’ve always welcome any kind of refactoring and cleanup in the past, I don’t see why you would want to guard it on having the tools / bot available before (or anything else FWIW).</div><div class="gmail_msg"><br class="gmail_msg"></div><div class="gmail_msg">Just `git log` and grep for NFC to get tons of such examples.</div></div></div></blockquote><div class=""><br class="">NFC changes are often refactoring to enabled further work</div></div></div></div></blockquote><div><br class=""></div><div>I doubt it is most of the case though, so it depends what you mean by “often”. And it does not invalidate that in many cases it is not the motivation.</div><br class=""><blockquote type="cite" class=""><div class=""><div dir="ltr" class=""><div class="gmail_quote"><div class=""> - I don't think they're exactly comparable to the patches under discussion here.<br class=""></div></div></div></div></blockquote><div><br class=""></div><div>That is still not the point: refactoring/cleanup for the sake of it has always been welcome.</div><div><br class=""></div><div>Ex: </div><div>- r298797 (terrible commit message by the way), </div><div>- For-range: r298680 <span style="color: rgba(0, 0, 0, 0.85098); font-family: 'Helvetica Neue';" class="">r296093 </span><span style="color: rgba(0, 0, 0, 0.85098); font-family: 'Helvetica Neue';" class="">r292471</span><span style="color: rgba(0, 0, 0, 0.85098); font-family: 'Helvetica Neue';" class=""> r</span>290617 …</div><div>- Include order: r253915 r254005 r225439 r222151</div><div>- r262374</div><div>- r274036</div><div>- …</div><div><br class=""></div><div>What I’d like Eugene to do though is to not mix fixing include order in the same commit as another class of improvements. Each “type” of improvements should deserve its own commits, it is easier to see and track the changes if the commit does not mix these together.</div><div><br class=""></div><div>— </div><div>Mehdi</div><div><br class=""></div></div></body></html>