llvm dev - Oct 2014 - [LLVMdev] lld coding style

On Wed, Oct 8, 2014 at 4:58 PM, Nick Kledzik <kledzik at apple.com> wrote:
> On Oct 8, 2014, at 9:34 AM, Chris Lattner <clattner at apple.com>
wrote:
> >> On Oct 8, 2014, at 1:55 AM, Renato Golin <renato.golin at
linaro.org>
> wrote:
> >> On 8 October 2014 05:25, Chris Lattner <clattner at
apple.com> wrote:
> >>>> Up until now the thread has been about “formatting”.  You
suggested
> renaming
> >>>> every variable in the project!
> >>>
> >>> If that's what it takes.
> >>
> >> If we're still talking about applying it to the HEAD, not the
whole
> >> history, I think it's feasible.
> >
> > Yep, to be clear, I mean one big change to head, not trying to rewrite
> history.
> >
> > -Chris
>
> In case it is not clear, the lld’s convention diverge from LLVM’s in two
> small ways that were designed to overcome bugs in the LLVM conventions.
> When the lld project first started up, I wrote the attached conventions doc
> to detail the reason why lld was different.
>
>
>
>
> Given the above reasons for the divergence, I would consider a mass
> variable renaming in lld sources would make the lld source base worse.
>
I see both sides of this, but ultimately would leave the decision to the
primary contributors to LLD. At this point, if Rui is in favor of it, I
think he should make it so (given that several of the other contributor
seem to be in favor as well).

That said...

> Yes, having uniforms coding styles is nice.  Therefore, I suggest we
> discuss a variable naming convention that fixes LLVM's problems and can
be
> adopted by all projects.

Sure, I actually have no problem with this.

I'm going to point out that one of the naming conventions used by LLD has
serious problems: naming variables with a leading underscore puts them
*way* too close to the reserved identifier space. Folks have accidentally
ended up with reserved names quite a few times already.

However, I care much less about the particular naming convention than that
we have a consistent naming convention. And changing LLD to LLVM's style
and then later changing LLVM's style (and all the subprojects) will not
appreciably increase the amount of churn required to the project as a
whole. So I don't think we should hold up progress in the pursuit of
perfection here.
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20141008/1dbcf3bd/attachment.html>

On Oct 8, 2014, at 7:04 PM, Chandler Carruth <chandlerc at google.com>
wrote:
> On Wed, Oct 8, 2014 at 4:58 PM, Nick Kledzik <kledzik at apple.com>
wrote:
> On Oct 8, 2014, at 9:34 AM, Chris Lattner <clattner at apple.com>
wrote:
> >> On Oct 8, 2014, at 1:55 AM, Renato Golin <renato.golin at
linaro.org> wrote:
> >> On 8 October 2014 05:25, Chris Lattner <clattner at
apple.com> wrote:
> >>>> Up until now the thread has been about “formatting”.  You
suggested renaming
> >>>> every variable in the project!
> >>>
> >>> If that's what it takes.
> >>
> >> If we're still talking about applying it to the HEAD, not the
whole
> >> history, I think it's feasible.
> >
> > Yep, to be clear, I mean one big change to head, not trying to rewrite
history.
> >
> > -Chris
> 
> In case it is not clear, the lld’s convention diverge from LLVM’s in two
small ways that were designed to overcome bugs in the LLVM conventions.  When
the lld project first started up, I wrote the attached conventions doc to detail
the reason why lld was different.
> 
> 
> 
> 
> Given the above reasons for the divergence, I would consider a mass
variable renaming in lld sources would make the lld source base worse.
> 
> I see both sides of this, but ultimately would leave the decision to the
primary contributors to LLD. At this point, if Rui is in favor of it, I think he
should make it so (given that several of the other contributor seem to be in
favor as well).
> 
> That said...
>  
> Yes, having uniforms coding styles is nice.  Therefore, I suggest we
discuss a variable naming convention that fixes LLVM's problems and can be
adopted by all projects.
> 
> Sure, I actually have no problem with this.
> 
> I'm going to point out that one of the naming conventions used by LLD
has serious problems: naming variables with a leading underscore puts them *way*
too close to the reserved identifier space. Folks have accidentally ended up
with reserved names quite a few times already.
The lld conventions for ivars is a leading underscore followed by a lowercase
letter.  The reserved identifiers are a leading underscore followed by an
uppercase letter.  There is no conflict.

On the other hand the LLVM conventions prevents the use of the -Wshadow which
catches real bugs.

> 
> However, I care much less about the particular naming convention than that
we have a consistent naming convention. And changing LLD to LLVM's style and
then later changing LLVM's style (and all the subprojects) will not
appreciably increase the amount of churn required to the project as a whole. So
I don't think we should hold up progress in the pursuit of perfection here.
So you are saying, let’s make lld’s code worse and maybe someday make lld’s and
llvm’s code better.

Why not hold off on changing lld until the broader naming convention is decided,
then change lld to that?  lld has been using these conventions for years.  What
is the rush?  Besides, if you strip the distinctive name from ivars, it will be
hard to automatically add that back.

-Nick

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

On Wed, Oct 8, 2014 at 7:20 PM, Nick Kledzik <kledzik at apple.com> wrote:
> The lld conventions for ivars is a leading underscore followed by a
> lowercase letter.  The reserved identifiers are a leading underscore
> followed by an uppercase letter.  There is no conflict.
>
And I didn't say that there was. They are *close*. Too close. People make
mistakes and get it wrong.

>
> On the other hand the LLVM conventions prevents the use of the -Wshadow
> which catches real bugs.
>
I don't think this is inherently true. I would have no trouble working to
fix existing shadows in LLVM and turn on the warning.

>
>
>
> However, I care much less about the particular naming convention than that
> we have a consistent naming convention. And changing LLD to LLVM's
style
> and then later changing LLVM's style (and all the subprojects) will not
> appreciably increase the amount of churn required to the project as a
> whole. So I don't think we should hold up progress in the pursuit of
> perfection here.
>
>
> So you are saying, let’s make lld’s code worse and maybe someday make
> lld’s and llvm’s code better.
>
That's a very LLD-centric way of looking at it.

I'm saying let's make the entire LLVM project better by being more
consistent between subprojects. Maybe someday we can make that consistent
state also a better state. But avoiding consistency until that arrives is,
in my opinion, blocking progress in the name of perfection. I would rather
incremental progress.

>
> Why not hold off on changing lld until the broader naming convention is
> decided, then change lld to that?  lld has been using these conventions for
> years.  What is the rush?
>
The rush is that more and more people would *like* to start contributing to
LLD. But they have to continually try to remember to deal with the
inconsistencies between the two codebases. That wastes peoples time and
discourages new contributors.

>  Besides, if you strip the distinctive name from ivars, it will be hard to
> automatically add that back.
>
The primary hope of changing all of LLVM's naming conventions is the ready
availability of Clang-based tools to manage the migration. Such tools don't
need distinctive names, they *know* which names are member variables.
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20141008/15a6871c/attachment.html>

On Wed, Oct 8, 2014 at 7:20 PM, Nick Kledzik <kledzik at apple.com> wrote:
> Sure, I actually have no problem with this.
>
> I'm going to point out that one of the naming conventions used by LLD
has
> serious problems: naming variables with a leading underscore puts them
> *way* too close to the reserved identifier space. Folks have accidentally
> ended up with reserved names quite a few times already.
>
> The lld conventions for ivars is a leading underscore followed by a
> lowercase letter.  The reserved identifiers are a leading underscore
> followed by an uppercase letter.  There is no conflict.
>
> On the other hand the LLVM conventions prevents the use of the -Wshadow
> which catches real bugs.
>
If people think this is a valuable warning, we should just fix it so that
it doesn't fire on this common pattern:
struct A {
  A(int x) : x(x) {}
  int x;
};

It should still fire on this pattern though:
struct A {
  A(int x) : x(x) { x++; } // I modified the parameter 'x', not the
member
'x'.
  int x;
};

We shouldn't create conventions just to work around bugs in a toolchain
that we control.

---

My two cents: LLVM's conventions around method names and local variable
names are widely ignored. For example, the IRBuilder uses leading capitals
for methods. Clang CodeGen does as well. Clang CodeGen also uses lots of
local variables with leading lower-case letters, which is against the
variable naming guidelines.

I think the takeaway from that is that lots of people don't actually
understand or agree with the LLVM naming standards in this area. I don't
think we should do a mass rename in LLD if we aren't willing to do a mass
rename across Clang internals, and so far no one has indicated a desire to
do that.

If we limit the scope of the proposed change to removing leading
underscores from fields, then that might be worth doing, but it's really up
to you folks. I just don't think we're ready for a function and variable
case unification yet.
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20141009/e6e8ab47/attachment.html>