llvm dev - Feb 2019 - RFC: changing variable naming rules in LLVM codebase

I had posted something in the code review but Chris suggested doing it 
here instead, which makes sense. Also I have to remember that the
discussion is specifically about spelling variables, not changing any
other spelling conventions.

Looking at names of "variables" there's reasonable support for
making
them visually more distinct from other kinds of names.  Regarding 
making data-member names distinct from local variables, some people 
don't see why it should matter, others find it helpful; given this 
neutral-to-helpful spectrum, going with the kind-of helpful convention 
seems preferable.

So:

- Local variables and formal parameters should be lower_case, with
  one exception: Variables/parameters that have lambda/function
  type should follow the function-name spelling rules.
- Class data members should have an "m_" prefix, so m_lower_case.
- Initialisms and other abbreviations would be considered words for 
  this purpose, so we have names such as:
    tli   // Local variable for TargetLoweringInfo
    m_cgm // Data member for CodeGenModule
- I don't have a good suggestion for file-static/global variables.
  Some people have suggested a "g_" prefix for globals, or possibly
  an "s_" prefix for class-static data.

Regarding the transition:

Some people have worried that the churn will cause blame issues.
I respectfully point out that in my own archaeology I have to deal
with lots of clang-format/indentation/other random semantically
meaningless refactoring, this is just one more.  Also the point is
not to optimize for git-blame but to optimize for reading what is
there at the moment.

A more focused and shorter transition period will create a lot of
short-term churn but get us to the good endpoint sooner.  Doing
conversions per-file or per-class (rather than per-function [too
small] or per-library [too big]) are probably the way to go.  
Given we are changing the names used for _data_, and we try to 
practice good data-hiding, the impact of the conversion of any 
given class *ought* to be reasonably confined.

If someone can make clang-tidy help with this, that's awesome.

I'm almost afraid to make the next suggestion, but here goes:
In more complicated/wide-impact cases, it would be possible to
stage a data-member name conversion into "small-bang" iterations
using a C++ tactic like this:
    class Foo {
      int m_bar; // The renamed member.
      int &Bar = m_bar; // TEMPORARY alias using the old name.
    };
This would have to be done sparingly and for good reason, such as
when the names are known across many components/subprojects and 
doing them all at once would be really too much. Someone would
have to commit to getting it all done and removing the aliases in
a reasonably short period of time.  Needing to do this trick would
be (IMO) strong evidence of poor software design and a place to
focus some refactoring effort.

HTH,
--paulr

> If someone can make clang-tidy help with this, that's awesome.
>
Drive By: There is clang-tidy's 'readability-identifier-naming' for
bulk-renaming and clang-rename (maybe clangd can help too) for manual
changes.

Especially clang-tidy-diff could be used to enforce at least new code
and a few modifications to these
tools might help doing the transition.

via llvm-dev <llvm-dev at lists.llvm.org> writes:
> Looking at names of "variables" there's reasonable support
for making
> them visually more distinct from other kinds of names.  Regarding 
> making data-member names distinct from local variables, some people 
> don't see why it should matter, others find it helpful; given this 
> neutral-to-helpful spectrum, going with the kind-of helpful convention 
> seems preferable.
There are drawbacks to the "m_" prefix notation, though.  It makes it
more work to move entities between class and local scope.  It's extra
typing, it's hard to pronounce, etc.

Now, these may be considered trivial complaints, but given that some of
them have already been raised, I think we need some discussion about it.
If there's overwhelming support for "m_" (and/or "g_"
etc.) then fine,
but if there's about even opinions either way, we ought to go with the
status quo, at least for now.
> So:
>
> - Local variables and formal parameters should be lower_case, with
>   one exception: Variables/parameters that have lambda/function
>   type should follow the function-name spelling rules.
"lower_case" is a pretty drastic change from CamelCase and camelCase.
So far the only argument for it I've seen is, "lldb uses it all over
the
place."  Having one subproject drive the standard for everything else
seems backward.  It's certainly possible I missed other opinions on this
though.
> - Initialisms and other abbreviations would be considered words for 
>   this purpose, so we have names such as:
>     tli   // Local variable for TargetLoweringInfo
>     m_cgm // Data member for CodeGenModule
I actually think we need something stronger.  Acronyms should be
discouraged unless it's absolutely clear what it is.  As has been
pointed out, "tli" means at least two common and very different
things:
"TargetLoweringInfo" and "TargetLibraryInfo."
> - I don't have a good suggestion for file-static/global variables.
>   Some people have suggested a "g_" prefix for globals, or
possibly
>   an "s_" prefix for class-static data.
Is this really helpful or does it just make the code more difficult to
work with?  I don't know since I've never used such conventions before.
But I'm hesitant to introduce really strong binding of scopes to names
because it make refactoring more tedious/difficult.
> Some people have worried that the churn will cause blame issues.
> I respectfully point out that in my own archaeology I have to deal
> with lots of clang-format/indentation/other random semantically
> meaningless refactoring, this is just one more.  Also the point is
> not to optimize for git-blame but to optimize for reading what is
> there at the moment.
I want to call out that we already have a policy on this in the current
coding standards:

  Our long term goal is for the entire codebase to follow the
  convention, but we explicitly do not want patches that do large-scale
  reformatting of existing code.

I could argue things either way, but we should realize that a mechanical
reformatting goes explicitly against current stated (and presumably
previously-agreed-upon) policy.

                               -David

On 2/22/2019 1:40 PM, David Greene via llvm-dev wrote:
> I actually think we need something stronger.  Acronyms should be
> discouraged unless it's absolutely clear what it is.
I think we should avoid terms that are both strict and subjective at the 
same time. (How do you test if something is absolutely clear?) Instead 
we should delegate resolving such ambiguities to the common sense of the 
author and the reviewers.

-Krzysztof

Am Fr., 22. Feb. 2019 um 09:59 Uhr schrieb via llvm-dev
<llvm-dev at lists.llvm.org>:
> Looking at names of "variables" there's reasonable support
for making
> them visually more distinct from other kinds of names.  Regarding
> making data-member names distinct from local variables, some people
> don't see why it should matter, others find it helpful; given this
> neutral-to-helpful spectrum, going with the kind-of helpful convention
> seems preferable.
>
> So:
>
> - Local variables and formal parameters should be lower_case, with
>   one exception: Variables/parameters that have lambda/function
>   type should follow the function-name spelling rules.
> - Class data members should have an "m_" prefix, so m_lower_case.
> - Initialisms and other abbreviations would be considered words for
>   this purpose, so we have names such as:
>     tli   // Local variable for TargetLoweringInfo
>     m_cgm // Data member for CodeGenModule
> - I don't have a good suggestion for file-static/global variables.
>   Some people have suggested a "g_" prefix for globals, or
possibly
>   an "s_" prefix for class-static data.
If the goal of prefixes is to visually distinguish different scopes:
Some IDEs/editors support semantic code highlighting [1], i.e. assign
different colors to local/global/static variables/members/parameters.
I know at least the following support this natively: KDevelop [1],
Visual Studio [2], Eclipse CDT [3]

Michael

[1]
https://zwabel.wordpress.com/2009/01/08/c-ide-evolution-from-syntax-highlighting-to-semantic-highlighting/
[2]
https://devblogs.microsoft.com/cppblog/first-look-at-the-new-c-ide-productivity-features-in-visual-studio-11/
[3]
https://help.eclipse.org/kepler/index.jsp?topic=%2Forg.eclipse.cdt.doc.user%2Freference%2Fcdt_u_c_editor_color.htm

On Fri, Feb 22, 2019 at 5:59 AM via llvm-dev <llvm-dev at lists.llvm.org>
wrote:
> - Local variables and formal parameters should be lower_case, with
>   one exception: Variables/parameters that have lambda/function
>   type should follow the function-name spelling rules.
>
I really dislike this exception. Callable objects are *objects* and locally
scoped, I would much prefer they look like variables.

Also, what about callable objects that aren't lambdas? Or that use
operator() for something other than emulating a function call?

I think the simple rule is superior.

- Initialisms and other abbreviations would be considered words
for
>   this purpose, so we have names such as:
>     tli   // Local variable for TargetLoweringInfo
>     m_cgm // Data member for CodeGenModule
>
Agreed.

> - I don't have a good suggestion for file-static/global variables.
>   Some people have suggested a "g_" prefix for globals, or
possibly
>   an "s_" prefix for class-static data.
>
These are rare enough that I'm not sure we need special rules for naming
them. They should also should typically be wrapped up in an actual API
limiting how widely they are referenced.

> Regarding the transition:
>
> Some people have worried that the churn will cause blame issues.
> I respectfully point out that in my own archaeology I have to deal
> with lots of clang-format/indentation/other random semantically
> meaningless refactoring, this is just one more.  Also the point is
> not to optimize for git-blame but to optimize for reading what is
> there at the moment.
>
> A more focused and shorter transition period will create a lot of
> short-term churn but get us to the good endpoint sooner.  Doing
> conversions per-file or per-class (rather than per-function [too
> small] or per-library [too big]) are probably the way to go.
> Given we are changing the names used for _data_, and we try to
> practice good data-hiding, the impact of the conversion of any
> given class *ought* to be reasonably confined.
>
I generally agree with this strategy. That said, I would still do it
somewhat lazily rather than eagerly, but batched much as you're suggesting.

>
> If someone can make clang-tidy help with this, that's awesome.
>
> I'm almost afraid to make the next suggestion, but here goes:
> In more complicated/wide-impact cases, it would be possible to
> stage a data-member name conversion into "small-bang" iterations
> using a C++ tactic like this:
>     class Foo {
>       int m_bar; // The renamed member.
>       int &Bar = m_bar; // TEMPORARY alias using the old name.
>     };
> This would have to be done sparingly and for good reason, such as
> when the names are known across many components/subprojects and
> doing them all at once would be really too much. Someone would
> have to commit to getting it all done and removing the aliases in
> a reasonably short period of time.  Needing to do this trick would
> be (IMO) strong evidence of poor software design and a place to
> focus some refactoring effort.
>
Honestly, I don't think we need to do this. We routinely make wide-ranging
API updates. If we need to do that, we do that.

What we *should* do is encourage anyone that before they decide to do this
to discuss it and see if there is a good way to hide this usage of a
variable name behind a better API and make *that* widespread change
instead. Then the name change is more local again.
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20190222/d44ab80c/attachment.html>

On Fri, Feb 22, 2019 at 9:40 AM David Greene via llvm-dev <
llvm-dev at lists.llvm.org> wrote:
> > So:
> >
> > - Local variables and formal parameters should be lower_case, with
> >   one exception: Variables/parameters that have lambda/function
> >   type should follow the function-name spelling rules.
>
> "lower_case" is a pretty drastic change from CamelCase and
camelCase.
> So far the only argument for it I've seen is, "lldb uses it all
over the
> place."  Having one subproject drive the standard for everything else
> seems backward.  It's certainly possible I missed other opinions on
this
> though.
>
This is not how I described my preference for it. I prefer it generally and
find it substantially easier to read code using it.

That said, I think the fact that LLDB uses it does contribute two
motivations:
1) It does show some subset of the community is familiar with it and has
found it to be a reasonably good convention for their usage.
2) It removes some cost of transition.

I wouldn't make the decision *because* of this, but I also don't think
we
should ignore these points.
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20190222/30ec9365/attachment.html>

> -----Original Message-----
> From: David Greene [mailto:dag at cray.com]
> Sent: Friday, February 22, 2019 2:40 PM
> To: Robinson, Paul
> Cc: asb at lowrisc.org; clattner at nondot.org; llvm-dev at lists.llvm.org;
> nd at arm.com
> Subject: Re: [llvm-dev] RFC: changing variable naming rules in LLVM
> codebase
> 
> via llvm-dev <llvm-dev at lists.llvm.org> writes:
> 
> > Looking at names of "variables" there's reasonable
support for making
> > them visually more distinct from other kinds of names.  Regarding
> > making data-member names distinct from local variables, some people
> > don't see why it should matter, others find it helpful; given this
> > neutral-to-helpful spectrum, going with the kind-of helpful convention
> > seems preferable.
> 
> There are drawbacks to the "m_" prefix notation, though.  It
makes it
> more work to move entities between class and local scope.  It's extra
> typing, it's hard to pronounce, etc.
IMO these are pretty feeble objections. The "m_" is silent, for
example.
Moving entities between class and local scope isn't all that common, and 
the small bit of extra typing provides clarity to your colleagues who 
like to know what data is ephemeral and what is persistent.  It's equally
true that using real words instead of shorthand abbreviations is more
typing, but we accept that for the clarity it brings.
> 
> Now, these may be considered trivial complaints, but given that some of
> them have already been raised, I think we need some discussion about it.
> If there's overwhelming support for "m_" (and/or
"g_" etc.) then fine,
> but if there's about even opinions either way, we ought to go with the
> status quo, at least for now.
I don't think "overwhelming" is an appropriate barrier to
adoption. I
hear people say "yes, this would make it easier for me to read the
code"
and on the other side "meh, I don't see the benefit"; however I do
*not*
see anyone saying "No, this really would interfere with my
code-reading."
> 
> > So:
> >
> > - Local variables and formal parameters should be lower_case, with
> >   one exception: Variables/parameters that have lambda/function
> >   type should follow the function-name spelling rules.
> 
> "lower_case" is a pretty drastic change from CamelCase and
camelCase.
> So far the only argument for it I've seen is, "lldb uses it all
over the
> place."  Having one subproject drive the standard for everything else
> seems backward.  It's certainly possible I missed other opinions on
this
> though.
You have. For example, here's a "significant improvement" comment:
http://lists.llvm.org/pipermail/llvm-dev/2019-February/130214.html

The debate suffers from being spread across multiple dev threads plus
a Phabricator review.  Not optimal, but that's what it is.  
> 
> > - Initialisms and other abbreviations would be considered words for
> >   this purpose, so we have names such as:
> >     tli   // Local variable for TargetLoweringInfo
> >     m_cgm // Data member for CodeGenModule
> 
> I actually think we need something stronger.  Acronyms should be
> discouraged unless it's absolutely clear what it is.  As has been
> pointed out, "tli" means at least two common and very different
things:
> "TargetLoweringInfo" and "TargetLibraryInfo."
That's a separate discussion and would be something to pursue regardless
of the other spelling conventions. Let's not drag that in here.  My
inclusion of this is based on "we find these things in names today, and
they aren't 'words' so what do we do with them" not on any
assessment
that they are good or bad on their own merits.
> 
> > - I don't have a good suggestion for file-static/global variables.
> >   Some people have suggested a "g_" prefix for globals, or
possibly
> >   an "s_" prefix for class-static data.
> 
> Is this really helpful or does it just make the code more difficult to
> work with?  I don't know since I've never used such conventions
before.
> But I'm hesitant to introduce really strong binding of scopes to names
> because it make refactoring more tedious/difficult.
I have, but in a project that had rather more global data than is good
for anyone. I put it out there for discussion, and thanks for doing
exactly that!
> 
> > Some people have worried that the churn will cause blame issues.
> > I respectfully point out that in my own archaeology I have to deal
> > with lots of clang-format/indentation/other random semantically
> > meaningless refactoring, this is just one more.  Also the point is
> > not to optimize for git-blame but to optimize for reading what is
> > there at the moment.
> 
> I want to call out that we already have a policy on this in the current
> coding standards:
> 
>   Our long term goal is for the entire codebase to follow the
>   convention, but we explicitly do not want patches that do large-scale
>   reformatting of existing code.
> 
> I could argue things either way, but we should realize that a mechanical
> reformatting goes explicitly against current stated (and presumably
> previously-agreed-upon) policy.
> 
>                                -David
Right, but my impression is that people who lived through the last update
to the spelling conventions are less enthused about a slow transition,
this time around, and that bit of the convention would necessarily also
be up for revision.

--paulr

Chandler wrote:
>> - Local variables and formal parameters should be lower_case, with
>>   one exception: Variables/parameters that have lambda/function
>>   type should follow the function-name spelling rules.
>
> I really dislike this exception. Callable objects are *objects* and
> locally scoped, I would much prefer they look like variables.
>
> Also, what about callable objects that aren't lambdas? Or that use
> operator() for something other than emulating a function call?
>
> I think the simple rule is superior. 
Hm I was probably mis-remembering your opinion from earlier, as this
was supposed to be accommodating it!  I'm happy to trash the exception.
--paulr

On Fri, Feb 22, 2019 at 12:39 PM Chandler Carruth via llvm-dev <
llvm-dev at lists.llvm.org> wrote:
> On Fri, Feb 22, 2019 at 5:59 AM via llvm-dev <llvm-dev at
lists.llvm.org>
> wrote:
>
>> - Local variables and formal parameters should be lower_case, with
>>   one exception: Variables/parameters that have lambda/function
>>   type should follow the function-name spelling rules.
>>
>
> I really dislike this exception. Callable objects are *objects* and
> locally scoped, I would much prefer they look like variables.
>
> Also, what about callable objects that aren't lambdas? Or that use
> operator() for something other than emulating a function call?
>
> I think the simple rule is superior.
>
> - Initialisms and other abbreviations would be considered words for
>>   this purpose, so we have names such as:
>>     tli   // Local variable for TargetLoweringInfo
>>     m_cgm // Data member for CodeGenModule
>>
>
> Agreed.
>
>
>> - I don't have a good suggestion for file-static/global variables.
>>   Some people have suggested a "g_" prefix for globals, or
possibly
>>   an "s_" prefix for class-static data.
>>
>
> These are rare enough that I'm not sure we need special rules for
naming
> them. They should also should typically be wrapped up in an actual API
> limiting how widely they are referenced.
>
>
>> Regarding the transition:
>>
>> Some people have worried that the churn will cause blame issues.
>> I respectfully point out that in my own archaeology I have to deal
>> with lots of clang-format/indentation/other random semantically
>> meaningless refactoring, this is just one more.  Also the point is
>> not to optimize for git-blame but to optimize for reading what is
>> there at the moment.
>>
>> A more focused and shorter transition period will create a lot of
>> short-term churn but get us to the good endpoint sooner.  Doing
>> conversions per-file or per-class (rather than per-function [too
>> small] or per-library [too big]) are probably the way to go.
>> Given we are changing the names used for _data_, and we try to
>> practice good data-hiding, the impact of the conversion of any
>> given class *ought* to be reasonably confined.
>>
>
> I generally agree with this strategy. That said, I would still do it
> somewhat lazily rather than eagerly, but batched much as you're
suggesting.
>
If we are going to update variable names in a batch, I'd like to nominate
lld as a starter project. It is a middle-sized LLVM subproject which
currently follows the today's LLVM naming convention, and because of its
size it shouldn't be too hard to convert the entire code base in a single
patch or a few patches.

>> If someone can make clang-tidy help with this, that's awesome.
>>
>> I'm almost afraid to make the next suggestion, but here goes:
>> In more complicated/wide-impact cases, it would be possible to
>> stage a data-member name conversion into "small-bang"
iterations
>> using a C++ tactic like this:
>>     class Foo {
>>       int m_bar; // The renamed member.
>>       int &Bar = m_bar; // TEMPORARY alias using the old name.
>>     };
>> This would have to be done sparingly and for good reason, such as
>> when the names are known across many components/subprojects and
>> doing them all at once would be really too much. Someone would
>> have to commit to getting it all done and removing the aliases in
>> a reasonably short period of time.  Needing to do this trick would
>> be (IMO) strong evidence of poor software design and a place to
>> focus some refactoring effort.
>>
>
> Honestly, I don't think we need to do this. We routinely make
wide-ranging
> API updates. If we need to do that, we do that.
>
> What we *should* do is encourage anyone that before they decide to do this
> to discuss it and see if there is a good way to hide this usage of a
> variable name behind a better API and make *that* widespread change
> instead. Then the name change is more local again.
> _______________________________________________
> 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/20190222/090e296a/attachment.html>

> On Feb 22, 2019, at 11:40 AM, David Greene <dag at cray.com> wrote:
> 
>> 
>> - Local variables and formal parameters should be lower_case, with
>>  one exception: Variables/parameters that have lambda/function
>>  type should follow the function-name spelling rules.
> 
> "lower_case" is a pretty drastic change from CamelCase and
camelCase.
> So far the only argument for it I've seen is, "lldb uses it all
over the
> place."  Having one subproject drive the standard for everything else
> seems backward.  It's certainly possible I missed other opinions on
this
> though.
FWIW, I agree.  LLDB has always been a vast outlier from the rest of the LLVM
project, owing to the project origins (which aren’t relevant to the discussions
at hand).  I don’t think that its conventions should be highly prioritized
unless they are obviously and objectively better.

In this case, the different between variable_names and TypeNames seems
completely indefensible.

-Chris

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

> On Feb 22, 2019, at 12:39 PM, Chandler Carruth <chandlerc at
gmail.com> wrote:
> 
> On Fri, Feb 22, 2019 at 5:59 AM via llvm-dev <llvm-dev at lists.llvm.org
<mailto:llvm-dev at lists.llvm.org>> wrote:
> - Local variables and formal parameters should be lower_case, with
>   one exception: Variables/parameters that have lambda/function
>   type should follow the function-name spelling rules.
> 
> I really dislike this exception. Callable objects are *objects* and locally
scoped, I would much prefer they look like variables.
Agreed, this is one of the baseline rationales for going with lowerCamel for
variables and functions, and UpperCamel for types.  C++ doesn’t have a
reasonable metatype system, and so this is a very useful fork between worlds. 
Functions are first class values that can be address-taken, passed around and
have other transformations applied to them, so they are fundamentally value-y.

-Chris

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