<div dir="ltr"><br><br><div class="gmail_quote"><div dir="ltr">On Fri, Apr 7, 2017 at 1:00 PM Mehdi Amini <<a href="mailto:mehdi.amini@apple.com">mehdi.amini@apple.com</a>> wrote:<br></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 Apr 7, 2017, at 11:30 AM, David Blaikie <<a href="mailto:dblaikie@gmail.com" class="gmail_msg" target="_blank">dblaikie@gmail.com</a>> wrote:</div><br class="m_-2552334888422395673Apple-interchange-newline gmail_msg"><div class="gmail_msg"><div dir="ltr" class="gmail_msg"><br class="gmail_msg"><br class="gmail_msg"><div class="gmail_quote gmail_msg"><div dir="ltr" class="gmail_msg">On Fri, Apr 7, 2017 at 11:18 AM Mehdi Amini <<a href="mailto:mehdi.amini@apple.com" class="gmail_msg" target="_blank">mehdi.amini@apple.com</a>> wrote:<br class="gmail_msg"></div><blockquote class="gmail_quote gmail_msg" 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_-2552334888422395673m_-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="gmail_msg"><br class="gmail_msg">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="gmail_msg"></div></div></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">What is “churn” and why is it “bad"?</div><div class="gmail_msg"><br class="gmail_msg"></div><div class="gmail_msg">The only thing is "git blame”, which is a very minor avoidance IMO (this is a tooling issue and is easily worked around).</div></div></div></blockquote><div><br>Mostly version control blame has been brought up in the past - whitespace is relatively easy to workaround in this regard (git at least has a flag for whitespace-ignorant blame) but wasn't considered sufficient to justify mass whitespace cleanup in LLVM previously so far as I know.<br><br>What sort of easy workaround do you have in mind for these non-whitespace changes?<br> </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"><div class="gmail_msg"> Massive Formatting are usually touching *most* of the file, and the benefit is sometimes minimal (single whitespace change?)</div></div></div></blockquote><div><br>Not sure I follow - if it's changing most of a file it's because most of the whitespace is changing, right? IF it's a single whitespace change, then it'd only modify that line.<br> </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"><div class="gmail_msg"> compare to the spread of the changes, which makes it “controversial” (I’d still do it, just like LLDB did recently).</div></div></div></blockquote><div><br>*nod* I'm less inclined to care about version history than others - I'm trying to reproduce general attitudes that have been expressed/motivating changes in the past.<br> </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"><div class="gmail_msg">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”.<br></div></div></div></blockquote><div><br>I don't really see them as so different, personally. The changes must be behavior preserving, though they might be easier to read - seems pretty similar to whitespace changes to me. At least I see these things as pretty close on one end of the NFC spectrum.<br> </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"><div class="gmail_msg"></div></div></div><div style="word-wrap:break-word" class="gmail_msg"><div class="gmail_msg"><div class="gmail_msg"><br class="gmail_msg"></div><blockquote type="cite" class="gmail_msg"><div class="gmail_msg"><div dir="ltr" class="gmail_msg"><div class="gmail_quote gmail_msg"><div class="gmail_msg"><br class="gmail_msg">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="gmail_msg"> </div><blockquote class="gmail_quote gmail_msg" 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="gmail_msg"><br class="gmail_msg">NFC changes are often refactoring to enabled further work</div></div></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 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></div></div><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"><div class="gmail_quote gmail_msg"><div class="gmail_msg"> - I don't think they're exactly comparable to the patches under discussion here.<br class="gmail_msg"></div></div></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">That is still not the point: refactoring/cleanup for the sake of it has always been welcome.</div><div class="gmail_msg"><br class="gmail_msg"></div><div class="gmail_msg">Ex: </div><div class="gmail_msg">- r298797 (terrible commit message by the way), </div><div class="gmail_msg">- For-range: r298680 <span style="color:rgba(0,0,0,0.85098);font-family:'Helvetica Neue'" class="gmail_msg">r296093 </span><span style="color:rgba(0,0,0,0.85098);font-family:'Helvetica Neue'" class="gmail_msg">r292471</span><span style="color:rgba(0,0,0,0.85098);font-family:'Helvetica Neue'" class="gmail_msg"> r</span>290617 …</div><div class="gmail_msg">- Include order: r253915 r254005 r225439 r222151</div><div class="gmail_msg">- r262374</div><div class="gmail_msg">- r274036</div><div class="gmail_msg">- …</div><div class="gmail_msg"><br class="gmail_msg"></div><div class="gmail_msg">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></div></blockquote><div><br>Sure, that seems good too.<br><br>But in general I'd still like to see this discussion on llvm-dev to get some consensus about what sort of cleanups/scale/approval/etc is desired. (eg: if these sort of wide cleanups are going to be done - eg: maybe approval on the type of change would be good/sufficient "Hey, is everyone good with fixing all the range-for in LLVM using clang-tidy" "yep" - great)<br><br>- David<br> </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"><div class="gmail_msg"><br class="gmail_msg"></div><div class="gmail_msg">— </div><div class="gmail_msg">Mehdi</div><div class="gmail_msg"><br class="gmail_msg"></div></div></div></blockquote></div></div>