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

[David Blaikie via llvm-commits]

On Fri, Apr 7, 2017 at 1:00 PM Mehdi Amini <mehdi.amini at apple.com> wrote:

> 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> wrote:
>
> On Mar 6, 2017, at 2:36 PM, David Blaikie via llvm-commits <
> 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?


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


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

- David


>
> —
> Mehdi
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170407/168a602b/attachment.html>