[llvm-dev] amount of camelCase refactoring causing some downstream overhead

Thu Feb 20 11:05:17 PST 2020

Hi,

Thanks for positioning some arguments with respect to upstream development,
appreciate it!

If `git blame` is what you're after, then it seems like we'd bundle
refactoring per "region of code" (function?) rather than multiple files
together: changes across files shouldn't create a long sequence of `git
blame` tracking right?

-- 
Mehdi

On Thu, Feb 20, 2020 at 10:33 AM Robinson, Paul <paul.robinson at sony.com>
wrote:

> Hi Mehdi!
>
>
>
> I think the value to upstream (of doing mass reformatting in fewer
> commits) has to do with the intrusion of nonfunctional commits into `git
> blame` kinds of research.  Every line that someone touches for a formatting
> reason necessarily obscures the history of functional changes in that block
> of code.  The fewer of those that people have to work around, the better.
> I admit this is a small concern, but it wasn’t so long ago that I was
> looking back at some history and had to wade through way more formatting
> commits than functional commits to get to the log message that told me what
> I needed to know.
>
>
>
> Some formatting changes just come with the territory, but if I have to
> deal with 1 or 2 instead of 5 or 8, it does improve my day.
>
>
>
> HTH,
>
> --paulr
>
>
>
> *From:* Mehdi AMINI <joker.eph at gmail.com>
> *Sent:* Thursday, February 20, 2020 1:16 PM
> *To:* Robinson, Paul <paul.robinson at sony.com>
> *Cc:* Philip Reames <listmail at philipreames.com>; Eric Christopher <
> echristo at gmail.com>; llvm-dev at lists.llvm.org; Valentin Churavy <
> v.churavy at gmail.com>
> *Subject:* Re: [llvm-dev] amount of camelCase refactoring causing some
> downstream overhead
>
>
>
>
>
>
>
> On Wed, Feb 19, 2020 at 12:10 PM Robinson, Paul via llvm-dev <
> llvm-dev at lists.llvm.org> wrote:
>
> Hi Philip,
>
>
>
> I think you might be reading more into the suggestion/discussion than is
> actually there.
>
>
>
>    - I do not want upstream developers "trying to be polite" if that
>    delays otherwise worthwhile work.
>
> Nobody suggested that.  It’s perfectly possible to “be polite” and still
> not delay worthwhile work.
>
>
>
>    - The current policy is "downstream is on their own".
>
> Nobody disputes that.
>
>
>
>    - There was nothing even remotely unreasonable done in the patch
>    series which triggered this discussion and I don't want any upstream
>    contributor coming to believe there was.
>
> I disagree with you about “nothing even remotely unreasonable” in that I
> think it was done slightly unreasonably, and I think it’s worthwhile to
> have other upstream contributors acknowledge that feeling.
>
>
>
>
>
> What I’m aware of is a series of 5 separate patches, each of which
> re-capitalized a select subset of APIs in the same area.  This fixing-up is
> **long overdue** and I’m very happy to see it done.  But I will dispute
> the “reasonableness” of doing it in 5 separate stages (plus a few other
> related cleanups that were not simply re-capitalizing existing names).
>
> The notion of “reviewable size” is not relevant, because these patches
> were not reviewed (in accordance with current policy, that this kind of
> mechanical fixup is “obvious” and does not require pre-commit review).
>
> The patches were not really very distinct, with a HUGE overlap in the set
> of affected files.
>
>
>
> We do like to minimize churn; this kind of fixup clearly qualifies as
> churn, but I respectfully submit that the way it was done does not *
> *minimize** the churn.
>
>
>
> And minimizing the churn is good for both the project and those of us who
> live downstream of it.
>
>
>
> If you ignore downstream users, and focus on upstream: what makes it the
> least risky and most convenient for the *upstream contributor* in this
> case? It seems they believed it was to split their patch up, which I would
> have done as well: if you can split it, then in most case you should.
>
> If the patches can land separately, to me their are distinct (they are not
> unrelated, but that's not a reason for merging them in a single commit).
> Just like David mentioned separately, I may have done this over a much
> longer series of individual commits.
>
>
>
> If you are arguing that the reason to not split these in separate patch is
> about downstream users, then I am with Philip: you start to creep up some
> expectations that are going against the current status quo in which (in my
> opinion) downstream shouldn't impact upstream development.
>
>
>
> Best,
>
>
>
> --
>
> Mehdi
>
>
>
>
>
>
>
>
>
> --paulr
>
>
>
> *From:* llvm-dev <llvm-dev-bounces at lists.llvm.org> *On Behalf Of *Philip
> Reames via llvm-dev
> *Sent:* Wednesday, February 19, 2020 2:07 PM
> *To:* Eric Christopher <echristo at gmail.com>
> *Cc:* llvm-dev <llvm-dev at lists.llvm.org>; Valentin Churavy <
> v.churavy at gmail.com>
> *Subject:* Re: [llvm-dev] amount of camelCase refactoring causing some
> downstream overhead
>
>
>
> Eric,
>
> I disagree.  Strongly.   I see the very fact we're engaging in the
> discussion of "just being polite" here as normalizing a proposed change in
> policy which has potentially profound negative consequences for the project
> long term.  I do not want upstream developers "trying to be polite" if that
> delays otherwise worthwhile work.  The current policy is "downstream is on
> their own".  There was nothing even remotely unreasonable done in the patch
> series which triggered this discussion and I don't want any upstream
> contributor coming to believe there was.
>
> Again, I'm open to carefully weighted proposals to change current policy.
> I also have a downstream repo which is kept up to date and I understand the
> pain point being raised.  I just want to be very careful to distinguish
> between existing status, and any proposed changes.  I want the proposed
> changes to be carefully weighed before being put into effect.
>
> Philip
>
> On 2/18/20 4:39 PM, Eric Christopher wrote:
>
> Hi Philip,
>
>
>
> While it's true we don't I think Valentin is reasonable in saying "hey,
> when people do this let's try to combine them if it makes sense". It's just
> being polite to everyone, especially if it doesn't risk the patches or
> upstream stability. I don't think there's a policy change being proposed,
> just a "hey, let's see what we can do in the future".
>
>
>
> -eric
>
>
>
> On Tue, Feb 18, 2020 at 4:05 PM Philip Reames via llvm-dev <
> llvm-dev at lists.llvm.org> wrote:
>
> Valentin,
>
> You are proposing to change existing policy.  Current policy is that we
> don't consider downstream *at all*.  Your proposal may seem reasonable - it
> may even *be* reasonable - but it is definitely a change from historical
> practice and must be considered as such.
>
> Philip
>
> On 2/18/20 3:03 PM, Valentin Churavy wrote:
>
> I don't think anyone is arguing to change longstanding policy. From a
> downstream perspective many small renaming changes do increase overhead for
> us.
>
>
>
> One thing that happens to downstream projects is that they support more
> than one LLVM version, we (JuliaLang) currently try to support latest
> stable + master.
>
>
>
> So for me the question is more, are renaming changes worth downstream
> projects not being able to test and provide feedback to upstream?  One way
> of mitigating that is to consciously schedule them just before a release
> and do them all in short succession.
>
>
>
> -V
>
>
>
>
>
> On Tue, Feb 18, 2020, 17:00 Philip Reames via llvm-dev <
> llvm-dev at lists.llvm.org> wrote:
>
> As others have said, our long standing policy has been that downstream
> projects must fend for themselves.  We're certainly not going to reverse
> that policy without careful discussion of the tradeoffs.
>
> I'm personally of the opinion that there could be a middle ground which
> allows upstream to move quickly while reducing headache for downstream
> projects.  Given I wear both hats, I know I'd certainly appreciate such
> a state.  However, it's important to state that such decisions would
> need to be carefully considered and would require some very careful
> drafting of proposal to balance the competing interests at hand.
>
> If anyone is curious, I'm happy to share some ideas offline on what
> starting points might be, but I have neither the time nor the interest
> to drive such a conversion on list.
>
> Philip
>
> On 2/18/20 1:37 AM, Ties Stuij via llvm-dev wrote:
> > During that variable renaming debate, there was a discussion about
> discussion about doing things all at once, piecemeal or not at all. An
> issue that wasn't really resolved I think. I had the impression that the
> efforts fizzled out a bit, and I thought this renaming was maybe related to
> that, but I'm neutral on if we should do variable renaming.
> >
> > All I'm asking as a kindness if we could be kind on poor downstream
> maintainers not on the issue of variable renaming at large, but on the
> micro level of not pushing 5/6 patches of this kind covering closely
> related functionality in two days but collating them in 1. I don't think
> that would slow down development, and I wanted to highlight the issue, as
> people might not be aware that they could save some pain in a simple way.
> Especially if indeed there is going to be a big renaming push and this
> would be a continuous thing.
> >
> > Cheers,
> > /Ties
> >
> > ________________________________________
> > From: Michael Kruse <llvmdev at meinersbur.de>
> > Sent: 17 February 2020 21:16
> > To: Ties Stuij
> > Cc: llvm-dev
> > Subject: Re: [llvm-dev] amount of camelCase refactoring causing some
> downstream overhead
> >
> > My understanding is that LLVM's general policy is to not let
> > downstream slow down upstream development. The C++ API explicitly
> > unstable.
> >
> > Note that we are even considering renaming variables globally:
> > https://lists.llvm.org/pipermail/llvm-dev/2019-September/134921.html
> >
> > Michael
> >
> > Am Mo., 17. Feb. 2020 um 06:04 Uhr schrieb Ties Stuij via llvm-dev
> > <llvm-dev at lists.llvm.org>:
> >> Hi there,
> >>
> >> At the end of last week we saw a number of commits go in that were
> camelCasing batches of MCStreamer::Emit* and AsmPrinter::Emit* functions.
> >>
> >> For example:
> >> - https://reviews.llvm.org/rG549b436beb4129854e729a3e1398f03429149691
> >> - https://reviews.llvm.org/rGa55daa146166353236aa528546397226bee9363b
> >> - https://reviews.llvm.org/rG0bc77a0f0d1606520c7ad0ea72c434661786a956
> >>
> >> Unfortunately all these individual commits trigger the same merge
> conflicts over and over again with our downstream repo, which takes us some
> manual intervention every time.
> >>
> >> I understand uniformity is a nice to have, but:
> >> 1 - is it worth it to do this work right now? I can remember the casing
> debate a few months back, which seems unrelated to this work which seems
> manual, but I'm unsure of the outcome.
> >> 2 - If this work should be done, it would be nice if all of the work is
> done in one batch, to save us some of the downstream overhead.
> >>
> >> Thanks
> >> /Ties
> >> _______________________________________________
> >> LLVM Developers mailing list
> >> llvm-dev at lists.llvm.org
> >> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
> > _______________________________________________
> > LLVM Developers mailing list
> > llvm-dev at lists.llvm.org
> > https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
> _______________________________________________
> LLVM Developers mailing list
> llvm-dev at lists.llvm.org
> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>
> _______________________________________________
> LLVM Developers mailing list
> llvm-dev at lists.llvm.org
> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>
> _______________________________________________
> LLVM Developers mailing list
> llvm-dev at lists.llvm.org
> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20200220/09f5d635/attachment-0001.html>