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

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 1:48 PM, Rui Ueyama via llvm-dev <llvm-dev at
lists.llvm.org> wrote:
> 
> 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.
This is a really great idea.  This would also give the ability to do an A/B
comparison with a specific set of examples.

The reason this matters to me is that we often get caught up on the theory of
various ideas, but seeing what it means in practice in full context can really
help sometimes.

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

Thanks very much for all the feedback. I've tried to collect the information
into a proposal for a transition plan: https://reviews.llvm.org/D59251.

-Michael
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20190312/8c8a6961/attachment.html>