llvm dev - Jul 2019 - RFC: changing variable naming rules in LLVM codebase & git-blame

On Jul 23, 2019, at 9:17 AM, JF Bastien via llvm-dev <llvm-dev at
lists.llvm.org> wrote:
>> On Jul 23, 2019, at 8:30 AM, James Y Knight via llvm-dev <llvm-dev
at lists.llvm.org> wrote:
>> 
>> As a very frequent explorer of history, I really don't think this
is
>> as big an issue as it may seem. Even absent refactorings, you often
>> run into the "wrong" commit when looking at blame (e.g.,
someone just
>> added a comma rather than actually changing the code you care about),
>> and have to look past that, to another previous commit.
>> 
>> Any interactive blame tool ought to have an easy way to do this. For
>> example, in emacs's annotation mode (which is what I use), you just
>> press 'a' with the cursor on the line in question to
re-annotate at
>> the commit previous to that.
> 
> You might very well be right, but your answer sounds like you *think* it’ll
work out. For a huge change like this I’d like to *know* for a fact that you’re
right. Changing variable naming will touch every single line except comments, so
it’s quite different from what we usually see today.
> 
> Artem mentions a new git feature that’ll do this automatically. Again:
sounds great, do we *know* that it’ll work?
Hi JF,

As discussed on other posts, there is good reason to believe that the
forthcoming git feature will cover this case.  We aren’t the only project that
has been held up by similar concerns about sweeping changes adversely affecting
history spelunking.  Similarly, as James Knight points out, this isn’t really
that big of a problem in practice even without the feature.

Sub projects in LLVM have gone through similar phases, including LLDB which
switched from 4 space indentation to 2 (as well as 80 columns iirc), which
affected literally everything.  This was unpopular, but was a good thing to do,
because commonality and consistency across the project is important.  I’m not
aware of a show-stopper problem even this massive of a change caused.

I think that Rui rolled this out in an incredibly great way with LLD,
incorporating a lot of community feedback and discussion, and (as you say) this
thread has accumulated many posts and a lot of discussion, so I don’t see the
concern about lack of communication.

The reason that I personally pivoted to believe that a “do everything all at
once in one commit generated by Rui’s script” approach was the right thing was
seeing how lld contributors were able to cleanly move forward even when they
have significant out of tree changes - they merged to the N-1 commit, then ran
Rui's script on *their* trees, and were caught up with commit N.  This seems
to have provided a very smooth transition path, where the conversion was merely
a bump in the road, not a major mountain to climb over.

In any case, I hope that we as a community don’t end up in a place where we are
afraid of doing sweeping changes that make the codebase better.  It is important
to continually reinvest in the health of the codebase, and it isn’t bad to
incentivize people to merge their changes upstream.  Being overwhelmingly afraid
of ‘breaking history’ is a path that leads stagnation and ultimate replacement
by newer technologies.  There are many other projects that have suffered this
fate, usually for a combination of reasons including things like this.

-Chris

> On Jul 28, 2019, at 4:08 PM, Chris Lattner <clattner at nondot.org>
wrote:
> 
> On Jul 23, 2019, at 9:17 AM, JF Bastien via llvm-dev <llvm-dev at
lists.llvm.org <mailto:llvm-dev at lists.llvm.org>> wrote:
>>> On Jul 23, 2019, at 8:30 AM, James Y Knight via llvm-dev
<llvm-dev at lists.llvm.org> wrote:
>>> 
>>> As a very frequent explorer of history, I really don't think
this is
>>> as big an issue as it may seem. Even absent refactorings, you often
>>> run into the "wrong" commit when looking at blame (e.g.,
someone just
>>> added a comma rather than actually changing the code you care
about),
>>> and have to look past that, to another previous commit.
>>> 
>>> Any interactive blame tool ought to have an easy way to do this.
For
>>> example, in emacs's annotation mode (which is what I use), you
just
>>> press 'a' with the cursor on the line in question to
re-annotate at
>>> the commit previous to that.
>> 
>> You might very well be right, but your answer sounds like you *think*
it’ll work out. For a huge change like this I’d like to *know* for a fact that
you’re right. Changing variable naming will touch every single line except
comments, so it’s quite different from what we usually see today.
>> 
>> Artem mentions a new git feature that’ll do this automatically. Again:
sounds great, do we *know* that it’ll work?
> 
> Hi JF,
> 
> As discussed on other posts, there is good reason to believe that the
forthcoming git feature will cover this case.
Hi Chris,

I’m happy there’s good reason to believe, but I’d like to know it’ll work. We’re
not in a rush, and it’s something I’d expect folks involved in moving this
proposal forward to demonstrate. Ideally it’s something that LLVM’s .git file
could even default `git blame` to: ignore the giant rewrite commit by default.

However, this is a small concern I’m trying to express… let’s ignore it for now
and focus on the rest:

> We aren’t the only project that has been held up by similar concerns about
sweeping changes adversely affecting history spelunking.  Similarly, as James
Knight points out, this isn’t really that big of a problem in practice even
without the feature.
> 
> Sub projects in LLVM have gone through similar phases, including LLDB which
switched from 4 space indentation to 2 (as well as 80 columns iirc), which
affected literally everything.  This was unpopular, but was a good thing to do,
because commonality and consistency across the project is important.  I’m not
aware of a show-stopper problem even this massive of a change caused.
> 
> I think that Rui rolled this out in an incredibly great way with LLD,
incorporating a lot of community feedback and discussion, and (as you say) this
thread has accumulated many posts and a lot of discussion, so I don’t see the
concern about lack of communication.
I think there’s lack of proper communication for this effort. The RFC is all
about variable naming, with 100+ responses. Sounds like a bikeshed I’ve happily
ignored, and I know many others have. Even if you don’t think I’m right, I’d
appreciate a separate RFC with details of what’s actually being proposed. Off
the top of my head I’d expect at least these questions answered:

What’s the final naming convention?
Will we have tools to auto-flag code that doesn’t follow it, and can auto-fix
it?
Will we clang-format everything while we’re at it?
Will we run clang modernizer to move code to C++11 / C++14 idioms while we’re
doing all this?
What’s the timeline for this change?
Is it just a single huge commit?
After the monorepo and GitHub move?
Is there a dev meeting roundtable scheduled?
What tooling exists to ease transition?
Out-of-tree LLVM backends are a normal thing. They use internal LLVM APIs that
should all be auto-updatable, has this been tried?
Some folks have significant non-upstream code. Have they signed up to remedy
that situation before the deadline (either by upstreaming or trying out
auto-update scripts)?

LLD and LLDB are indeed good small-scale experiments. However, I think the rest
of the project is quite different in the impact such a change would have. LLVM
and clang expose many more C++ APIs, and have many more out-of-tree changes
(either on top of upstream, or in sub-folders such as backends or clang tools).
They also have many more contributors affected, and not all those contributors
have the same constraints, making this much more complex. So far this discussion
hasn’t seemed to care about these concerns, and I’m worried we’re about to burn
a bunch of bridges. Maybe I missed this part of the discussion in the 100+
emails! Sorry if I did… but again, a simple updated RFC would solve everything.

> The reason that I personally pivoted to believe that a “do everything all
at once in one commit generated by Rui’s script” approach was the right thing
was seeing how lld contributors were able to cleanly move forward even when they
have significant out of tree changes - they merged to the N-1 commit, then ran
Rui's script on *their* trees, and were caught up with commit N.  This seems
to have provided a very smooth transition path, where the conversion was merely
a bump in the road, not a major mountain to climb over.
That sounds fine to me. I just want to make sure everyone knows they’re about to
have to climb a little hill. Surprise hills are the worst.

> In any case, I hope that we as a community don’t end up in a place where we
are afraid of doing sweeping changes that make the codebase better.
Sure the current naming is ugly and inconsistent, but honestly I don’t really
care about naming. The one upside I see from this change is the same
clang-format has brought: we can finally stop picking nits and arguing about
stuff that doesn’t matter.

I agree that we should be able to make sweeping changes. I’m saying that I don’t
think the direction I see us going in right now seems like it’ll work out well,
and it seems easy to address my worries.

> It is important to continually reinvest in the health of the codebase, and
it isn’t bad to incentivize people to merge their changes upstream.
Agreed. However, as you know very well, merging upstream changes isn’t something
that’s done over a weekend. First people have to know that a large change is
coming and they probably should plan for upstreaming. Second we should give
these folks enough time to do the upstreaming, and leave them enough slack in
case they fall behind.

> Being overwhelmingly afraid of ‘breaking history’ is a path that leads
stagnation and ultimate replacement by newer technologies.  There are many other
projects that have suffered this fate, usually for a combination of reasons
including things like this.
Again, this concern is small compared to the others I’ve expressed. Let’s have
it as one item on the list of things we want answers to before moving forward. I
think the rest of what I’m raising is much more important.

-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20190729/58450e3c/attachment.html>

Am Mo., 29. Juli 2019 um 12:59 Uhr schrieb JF Bastien via llvm-dev
<llvm-dev at lists.llvm.org>:
> I think there’s lack of proper communication for this effort. The RFC is
all about variable naming, with 100+ responses. Sounds like a bikeshed I’ve
happily ignored, and I know many others have. Even if you don’t think I’m right,
I’d appreciate a separate RFC with details of what’s actually being proposed.
Off the top of my head I’d expect at least these questions answered:
>
> What’s the final naming convention?
> Will we have tools to auto-flag code that doesn’t follow it, and can
auto-fix it?
> Will we clang-format everything while we’re at it?
> Will we run clang modernizer to move code to C++11 / C++14 idioms while
we’re doing all this?
> What’s the timeline for this change?
> Is it just a single huge commit?
> After the monorepo and GitHub move?
> Is there a dev meeting roundtable scheduled?
> What tooling exists to ease transition?
> Out-of-tree LLVM backends are a normal thing. They use internal LLVM APIs
that should all be auto-updatable, has this been tried?
> Some folks have significant non-upstream code. Have they signed up to
remedy that situation before the deadline (either by upstreaming or trying out
auto-update scripts)?
* Do we also change the remaining method names starting with and
uppercase letter (Such as IRBuilder::*)?

Michael

On Jul 29, 2019, at 10:58 AM, JF Bastien <jfbastien at apple.com>
wrote:
>> I think that Rui rolled this out in an incredibly great way with LLD,
incorporating a lot of community feedback and discussion, and (as you say) this
thread has accumulated many posts and a lot of discussion, so I don’t see the
concern about lack of communication.
> 
> I think there’s lack of proper communication for this effort. The RFC is
all about variable naming, with 100+ responses. Sounds like a bikeshed I’ve
happily ignored, and I know many others have. Even if you don’t think I’m right,
I’d appreciate a separate RFC with details of what’s actually being proposed.
Off the top of my head I’d expect at least these questions answered:
> 
> What’s the final naming convention?
> Will we have tools to auto-flag code that doesn’t follow it, and can
auto-fix it?
> Will we clang-format everything while we’re at it?
> Will we run clang modernizer to move code to C++11 / C++14 idioms while
we’re doing all this?
> What’s the timeline for this change?
> Is it just a single huge commit?
> After the monorepo and GitHub move?
> Is there a dev meeting roundtable scheduled?
> What tooling exists to ease transition?
> Out-of-tree LLVM backends are a normal thing. They use internal LLVM APIs
that should all be auto-updatable, has this been tried?
> Some folks have significant non-upstream code. Have they signed up to
remedy that situation before the deadline (either by upstreaming or trying out
auto-update scripts)?
> 
> LLD and LLDB are indeed good small-scale experiments. However, I think the
rest of the project is quite different in the impact such a change would have.
LLVM and clang expose many more C++ APIs, and have many more out-of-tree changes
(either on top of upstream, or in sub-folders such as backends or clang tools).
They also have many more contributors affected, and not all those contributors
have the same constraints, making this much more complex. So far this discussion
hasn’t seemed to care about these concerns, and I’m worried we’re about to burn
a bunch of bridges. Maybe I missed this part of the discussion in the 100+
emails! Sorry if I did… but again, a simple updated RFC would solve everything.
Thanks for the detailed list here.  I have no idea what the status of most of
these are - it sounds like you’re generally asking “what is the plan?” beyond
LLD.  :-)

Rui, what are your thoughts on next steps?  LLDB seems like a logical step,
particularly because it uses its own naming convention that is completely unlike
the rest of the project.

-Chris




> 
>> The reason that I personally pivoted to believe that a “do everything
all at once in one commit generated by Rui’s script” approach was the right
thing was seeing how lld contributors were able to cleanly move forward even
when they have significant out of tree changes - they merged to the N-1 commit,
then ran Rui's script on *their* trees, and were caught up with commit N. 
This seems to have provided a very smooth transition path, where the conversion
was merely a bump in the road, not a major mountain to climb over.
> 
> That sounds fine to me. I just want to make sure everyone knows they’re
about to have to climb a little hill. Surprise hills are the worst.
> 
> 
>> In any case, I hope that we as a community don’t end up in a place
where we are afraid of doing sweeping changes that make the codebase better.
> 
> Sure the current naming is ugly and inconsistent, but honestly I don’t
really care about naming. The one upside I see from this change is the same
clang-format has brought: we can finally stop picking nits and arguing about
stuff that doesn’t matter.
> 
> I agree that we should be able to make sweeping changes. I’m saying that I
don’t think the direction I see us going in right now seems like it’ll work out
well, and it seems easy to address my worries.
> 
> 
>> It is important to continually reinvest in the health of the codebase,
and it isn’t bad to incentivize people to merge their changes upstream.
> 
> Agreed. However, as you know very well, merging upstream changes isn’t
something that’s done over a weekend. First people have to know that a large
change is coming and they probably should plan for upstreaming. Second we should
give these folks enough time to do the upstreaming, and leave them enough slack
in case they fall behind.
> 
> 
>> Being overwhelmingly afraid of ‘breaking history’ is a path that leads
stagnation and ultimate replacement by newer technologies.  There are many other
projects that have suffered this fate, usually for a combination of reasons
including things like this.
> 
> Again, this concern is small compared to the others I’ve expressed. Let’s
have it as one item on the list of things we want answers to before moving
forward. I think the rest of what I’m raising is much more important.
> 
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20190729/4d9768e2/attachment-0001.html>