
<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 2:11 PM, 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" style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><br class=""><br class=""><div class="gmail_quote"><div dir="ltr" class="">On Fri, Apr 7, 2017 at 1:59 PM 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: 0px 0px 0px 0.8ex; border-left-width: 1px; border-left-style: solid; border-left-color: rgb(204, 204, 204); padding-left: 1ex;"><div class="gmail_msg" style="word-wrap: break-word;"><div class="gmail_msg"><blockquote type="cite" class="gmail_msg"><div class="gmail_msg">On Apr 7, 2017, at 1:40 PM, David Blaikie <<a href="mailto:dblaikie@gmail.com" class="gmail_msg" target="_blank">dblaikie@gmail.com</a>> wrote:</div><br class="m_5994061264597527662Apple-interchange-newline gmail_msg"><div class="gmail_msg"><div dir="ltr" class="gmail_msg" style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px;"><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 1:00 PM 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: 0px 0px 0px 0.8ex; border-left-width: 1px; border-left-style: solid; border-left-color: rgb(204, 204, 204); padding-left: 1ex;"><div class="gmail_msg" style="word-wrap: break-word;"><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="gmail_msg m_5994061264597527662m_-2552334888422395673Apple-interchange-newline"><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: 0px 0px 0px 0.8ex; border-left-width: 1px; border-left-style: solid; border-left-color: rgb(204, 204, 204); padding-left: 1ex;"><div class="gmail_msg" style="word-wrap: break-word;"><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_5994061264597527662m_-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 class="gmail_msg" style="word-wrap: break-word;"><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 class="gmail_msg" style="word-wrap: break-word;"><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 class="gmail_msg"><br class="gmail_msg">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 class="gmail_msg"><br class="gmail_msg">What sort of easy workaround do you have in mind for these non-whitespace changes?<br class="gmail_msg"></div></div></div></div></blockquote><div class="gmail_msg"><br class="gmail_msg"></div></div></div><div class="gmail_msg" style="word-wrap: break-word;"><div class="gmail_msg"><div class="gmail_msg">Recursive blame. Example: <a href="https://github.com/llvm-project/llvm-project/blame/master/llvm/include/llvm/DebugInfo/DIContext.h" class="gmail_msg" target="_blank">https://github.com/llvm-project/llvm-project/blame/master/llvm/include/llvm/DebugInfo/DIContext.h</a></div><div class="gmail_msg"><br class="gmail_msg"></div><div class="gmail_msg">Line 20 is: #include <cassert></div><div class="gmail_msg">The associated blame entry is: [DebugInfo] Fix some Clang-tidy modernize-use-default, modernize-use-…</div><div class="gmail_msg">Next to the blame entry is “5 months ago”, and right next to it an icon that allows you to jump to the blame before this change.</div><div class="gmail_msg">(this is applicable to any change, whitespace or non-whitespace)</div></div></div></blockquote><div class=""><br class="">Nice UI liek that is certainly handy - though seems like the sort of work that was generally thought of as "too much" when discussing whitespace cleanup. (it's certainly a matter of degrees - a somewhat linear cost the more lines of code have this, and the more times they have it in their history)<br class=""></div></div></div></div></blockquote><div><br class=""></div><div>Second tool is <a href="https://commondatastorage.googleapis.com/chrome-infra-docs/flat/depot_tools/docs/html/git-hyper-blame.html" class="">https://commondatastorage.googleapis.com/chrome-infra-docs/flat/depot_tools/docs/html/git-hyper-blame.html</a></div><div>But it is not clear to me how to maintain this? We could manually add hashes to a committed file, but we’re still committing to SVN and the workflow isn’t clear.</div><div><br class=""></div><div><br class=""></div><br class=""><blockquote type="cite" class=""><div class=""><div dir="ltr" style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><div class="gmail_quote"><div class=""> </div><blockquote class="gmail_quote" style="margin: 0px 0px 0px 0.8ex; border-left-width: 1px; border-left-style: solid; border-left-color: rgb(204, 204, 204); padding-left: 1ex;"><div class="gmail_msg" style="word-wrap: break-word;"><div class="gmail_msg"><br class="gmail_msg"><blockquote type="cite" class="gmail_msg"><div class="gmail_msg"><div dir="ltr" class="gmail_msg" style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px;"><div class="gmail_quote gmail_msg"><div class="gmail_msg"> </div><blockquote class="gmail_quote gmail_msg" style="margin: 0px 0px 0px 0.8ex; border-left-width: 1px; border-left-style: solid; border-left-color: rgb(204, 204, 204); padding-left: 1ex;"><div class="gmail_msg" style="word-wrap: break-word;"><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 class="gmail_msg"><br class="gmail_msg">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 class="gmail_msg"></div></div></div></div></blockquote><div class="gmail_msg"><br class="gmail_msg"></div></div></div><div class="gmail_msg" style="word-wrap: break-word;"><div class="gmail_msg"><div class="gmail_msg">Right: clang-formatting the full codebase is unlikely to change a single line per file but instead would touch massive hunks. At least I believe that’s the expectation that justifies the pushback on such practice.</div></div></div></blockquote><div class=""><br class="">Then I'm not sure I understand your "the benefit is sometimes minimal (single whitespace change?)”.<br class=""></div></div></div></div></blockquote><div><br class=""></div><div>I meant: I don’t really care about the benefit of changing a single whitespace like in `for(..)` -> `for (…)` ; while I care more about turning the for loop from `for (ilist<Instruction>::const_iterator It = BB.begin(); It != BB.end(); ++It)` to `for (const auto &Inst : BB)`.</div><div><br class=""></div><br class=""><blockquote type="cite" class=""><div class=""><div dir="ltr" style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><div class="gmail_quote"><div class=""> </div><blockquote class="gmail_quote" style="margin: 0px 0px 0px 0.8ex; border-left-width: 1px; border-left-style: solid; border-left-color: rgb(204, 204, 204); padding-left: 1ex;"><div class="gmail_msg" style="word-wrap: break-word;"><div class="gmail_msg"><div class="gmail_msg"><br class="gmail_msg"></div><br class="gmail_msg"><blockquote type="cite" class="gmail_msg"><div class="gmail_msg"><div dir="ltr" class="gmail_msg" style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px;"><div class="gmail_quote gmail_msg"><div class="gmail_msg"> </div><blockquote class="gmail_quote gmail_msg" style="margin: 0px 0px 0px 0.8ex; border-left-width: 1px; border-left-style: solid; border-left-color: rgb(204, 204, 204); padding-left: 1ex;"><div class="gmail_msg" style="word-wrap: break-word;"><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 class="gmail_msg"><br class="gmail_msg">*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 class="gmail_msg"> </div><blockquote class="gmail_quote gmail_msg" style="margin: 0px 0px 0px 0.8ex; border-left-width: 1px; border-left-style: solid; border-left-color: rgb(204, 204, 204); padding-left: 1ex;"><div class="gmail_msg" style="word-wrap: break-word;"><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 class="gmail_msg"></div></div></div></blockquote><div class="gmail_msg"><br class="gmail_msg">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 class="gmail_msg"> </div><blockquote class="gmail_quote gmail_msg" style="margin: 0px 0px 0px 0.8ex; border-left-width: 1px; border-left-style: solid; border-left-color: rgb(204, 204, 204); padding-left: 1ex;"><div class="gmail_msg" style="word-wrap: break-word;"><div class="gmail_msg"><div class="gmail_msg"></div></div></div><div class="gmail_msg" style="word-wrap: break-word;"><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: 0px 0px 0px 0.8ex; border-left-width: 1px; border-left-style: solid; border-left-color: rgb(204, 204, 204); padding-left: 1ex;"><div class="gmail_msg" style="word-wrap: break-word;"><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 class="gmail_msg" style="word-wrap: break-word;"><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 class="gmail_msg" style="word-wrap: break-word;"><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 class="gmail_msg" style="word-wrap: break-word;"><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 class="gmail_msg" style="word-wrap: break-word;"><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 class="gmail_msg m_5994061264597527662Apple-converted-space"> </span><span class="gmail_msg" style="font-family: 'Helvetica Neue';">r296093 </span><span class="gmail_msg" style="font-family: 'Helvetica Neue';">r292471</span><span class="gmail_msg" style="font-family: 'Helvetica Neue';"> 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 class="gmail_msg"><br class="gmail_msg">Sure, that seems good too.<br class="gmail_msg"><br class="gmail_msg">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 class="gmail_msg"></div></div></div></div></blockquote><div class="gmail_msg"><br class="gmail_msg"></div></div></div><div class="gmail_msg" style="word-wrap: break-word;"><div class="gmail_msg"><div class="gmail_msg">I believe you're trying to set a precedent: I believe such cleanup *are* currently welcome by default and you’re actually making these require a buy-in on llvm-dev. So my view is it that it is up to you to ask llvm-dev to change the status-quo and make them non-welcome by default, as I believe they currently are (cf commit list above).</div><div class="gmail_msg"><br class="gmail_msg"></div><div class="gmail_msg">As of today, I don’t feel I have to ask permission to fix/cleanup/refactor anything in the codebase (and apparently none of the author of the commits I mentioned above felt they have to, and the fact that a tool is used to find a for-range loop to update instead of doing it manually does not seem relevant to me).</div></div></div></blockquote><div class=""><br class="">Automated/widespread changes versus localized ones have generally made a difference (</div></div></div></div></blockquote><div><br class=""></div><br class=""><blockquote type="cite" class=""><div class=""><div dir="ltr" style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><div class="gmail_quote"><div class="">whitespace formatting a single file when it's about to be a major refactor to the whole file is one thing</div></div></div></div></blockquote><div><br class=""></div><div>This ^ is not what happened in the commits I mentioned above.</div><div><br class=""></div><div>— </div><div>Mehdi</div><div><br class=""></div></div></body></html>