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

Fri Apr 7 14:24:42 PDT 2017

> On Apr 7, 2017, at 2:11 PM, David Blaikie <dblaikie at gmail.com> wrote:
> 
> 
> 
> On Fri, Apr 7, 2017 at 1:59 PM Mehdi Amini <mehdi.amini at apple.com <mailto:mehdi.amini at apple.com>> wrote:
>> On Apr 7, 2017, at 1:40 PM, David Blaikie <dblaikie at gmail.com <mailto:dblaikie at gmail.com>> wrote:
>> 
>> 
>> 
>> On Fri, Apr 7, 2017 at 1:00 PM Mehdi Amini <mehdi.amini at apple.com <mailto:mehdi.amini at apple.com>> wrote:
>>> On Apr 7, 2017, at 11:30 AM, David Blaikie <dblaikie at gmail.com <mailto: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).
>> 
>> 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.
>> 
>> What sort of easy workaround do you have in mind for these non-whitespace changes?
> 
> Recursive blame. Example: https://github.com/llvm-project/llvm-project/blame/master/llvm/include/llvm/DebugInfo/DIContext.h <https://github.com/llvm-project/llvm-project/blame/master/llvm/include/llvm/DebugInfo/DIContext.h>
> 
> Line 20 is: #include <cassert>
> The associated blame entry is: [DebugInfo] Fix some Clang-tidy modernize-use-default, modernize-use-…
> 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.
> (this is applicable to any change, whitespace or non-whitespace)
> 
> 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)

Second tool is https://commondatastorage.googleapis.com/chrome-infra-docs/flat/depot_tools/docs/html/git-hyper-blame.html <https://commondatastorage.googleapis.com/chrome-infra-docs/flat/depot_tools/docs/html/git-hyper-blame.html>
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.

>  
> 
>>  
>> Massive Formatting are usually touching *most* of the file, and the benefit is sometimes minimal (single whitespace change?)
>> 
>> 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.
> 
> 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.
> 
> Then I'm not sure I understand your "the benefit is sometimes minimal (single whitespace change?)”.

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

>  
> 
> 
>>  
>> compare to the spread of the changes, which makes it “controversial” (I’d still do it, just like LLDB did recently).
>> 
>> *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.
>>  
>> 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”.
>> 
>> 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.
>>  
>> 
>>> 
>>> 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.
>> 
>> Sure, that seems good too.
>> 
>> 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)
> 
> 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).
> 
> 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).
> 
> Automated/widespread changes versus localized ones have generally made a difference (

> whitespace formatting a single file when it's about to be a major refactor to the whole file is one thing

This ^ is not what happened in the commits I mentioned above.

— 
Mehdi

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