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

Fri Apr 7 14:11:22 PDT 2017

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

> On Apr 7, 2017, at 1:40 PM, David Blaikie <dblaikie at gmail.com> wrote:
>
>
>
> 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?
>
>
> Recursive blame. Example:
> 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)

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

>
>
>
>
> 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 - whitespace reformatting the
whole project is another, for example).

- David

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