[llvm] r296413 - [DebugInfo] Fix some Clang-tidy modernize and Include What You Use warnings; other minor fixes (NFC).

[Mehdi Amini via llvm-commits]

> On Apr 7, 2017, at 11:30 AM, David Blaikie <dblaikie at gmail.com> wrote:
> 
> 
> 
> On Fri, Apr 7, 2017 at 11:18 AM Mehdi Amini <mehdi.amini at apple.com <mailto:mehdi.amini at apple.com>> wrote:
>> On Mar 6, 2017, at 2:36 PM, David Blaikie via llvm-commits <llvm-commits at lists.llvm.org <mailto:llvm-commits at lists.llvm.org>> wrote:
>> 
>> 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.
> 
> I don’t agree with this statement: I don’t believe we avoided *cleanup* but instead we avoided *mass formatting* changes.
> 
> 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.

What is “churn” and why is it “bad"?

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).

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”.

> 
> To me that motivation potentially applies to more than just whitespace & some of these changes seem like they maybe fall under a similar categorization.
>  
> 
>> 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)
>> 
>> Please start an llvm-dev thread and get some kind of closure on the best strategy before continuing with these sort of changes.
> 
> 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).
> 
> Just `git log` and grep for NFC to get tons of such examples.
> 
> NFC changes are often refactoring to enabled further work

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.

> - I don't think they're exactly comparable to the patches under discussion here.

That is still not the point: refactoring/cleanup for the sake of it has always been welcome.

Ex: 
- r298797 (terrible commit message by the way), 
- For-range: r298680 r296093 r292471 r290617 …
- Include order: r253915 r254005 r225439 r222151
- r262374
- r274036
- …

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.

— 
Mehdi

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170407/937b2d68/attachment.html>