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

On Mon, 11 Feb 2019 at 23:20, Philip Reames via llvm-dev
<llvm-dev at lists.llvm.org> wrote:
>
> I don't care about the convention, but I'm really not sure it's
worth the churn which would result in the code base.  The hurtle which needs
cleared here is not "is it a better naming style", but "is the
disruption implied by changing to the new convention justified".  To be
clear, not opposed, just hesitant.
I have the same concern. The whole advantage of a common coding
convention is consistency. There are exceptions, but the vast majority
of LLVM and Clang code I've read does indeed stick to the current
CamelCase convention. Unless there's a plan for conversion then the
practical impact of the naming convention change is that the codebase
will be a muddle of mixed conventions for years. That seems like a
regression, even if camelBack is a better convention.

On a more practical note, if the intent is to move to camelBack I
think it would be worth adding an example to the coding standard for
handling an acronym. e.g. is it the intent that TTI would become tti.

Best,

Alex

Regarding a plan for conversion, I'm keen to avoid perfect being the enemy
of better.

Privately, people I've spoken with have told me that they're opposed to
a large scale conversion. Reasons given include breaking git blame, and creating
needless merge conflicts. I might be wrong, but the evidence I've seen
suggests that it's going to be very hard to get consensus on a conversion.

So what's worse: inconsistent capitalization or keeping a convention that
discourages good naming?

Taking my previous example [1]:

InnerLoopVectorizer LB(L, PSE, LI, DT, TLI, TTI, AC, ORE, VF.Width, IC,
  &LVL, &CM);

If we imagine that over time it evolves such that 50% of the variables have been
renamed to camelBack versions of the type names, then it will look like this:

InnerLoopVectorizer LB(loop, PSE, loopInfo, DT, targetLibraryInfo, TTI,
  assumptionCache, ORE, vectorizationFactor.Width, IC,
  &loopVectorizationLegality, &CM);

Yes it's inconsistent, but IMHO it still conveys so much more information
than the original that the benefit greatly outweighs the cost.

So is the disruption implied by changing to the new convention justified? I
think so.

[1]
https://github.com/llvm/llvm-project/blob/ec0263027811f48b907224ede0dd24c33c1c7507/llvm/lib/Transforms/Vectorize/LoopVectorize.cpp#L7433

On Mon, Feb 18, 2019 at 2:16 AM Michael Platings via llvm-dev
<llvm-dev at lists.llvm.org> wrote:
> Regarding a plan for conversion, I'm keen to avoid perfect being the
enemy of better.
>
> Privately, people I've spoken with have told me that they're
opposed to a large scale conversion. Reasons given include breaking git blame,
and creating needless merge conflicts. I might be wrong, but the evidence
I've seen suggests that it's going to be very hard to get consensus on a
conversion.
>
> So what's worse: inconsistent capitalization or keeping a convention
that discourages good naming?
>
> Taking my previous example [1]:
>
> InnerLoopVectorizer LB(L, PSE, LI, DT, TLI, TTI, AC, ORE, VF.Width, IC,
>   &LVL, &CM);
>
> If we imagine that over time it evolves such that 50% of the variables have
been renamed to camelBack versions of the type names, then it will look like
this:
>
> InnerLoopVectorizer LB(loop, PSE, loopInfo, DT, targetLibraryInfo, TTI,
>   assumptionCache, ORE, vectorizationFactor.Width, IC,
>   &loopVectorizationLegality, &CM);
I find myself less productive in a codebase with inconsistent styling
like you show above because it is more difficult to "guess" the name
of a variable.  E.g. is the LoopInfo parameter named LI or loopInfo?
I'll have to double check to be sure, which adds an extra step.

So maybe a gradual transition plan could be to allow these upper case
acronyms for specific classes?  For instance we could start by
designating a set of "common" classes like Function, BasicBlock
DominatorTree, LoopInfo, ScalarEvolution whose instances would
instances still be called F, BB, DT, LI and SE, but mandate all other
classes should use the new camelCase convention to name their
instances?  I think this helps readability since the reader will know
that "BB" is always the basic block, "F" is always the
function etc.
And I don't have the "guessing" problem I mentioned above since
VectorationFactor instances are always "vectorizationFactor" and
LoopInfo instances are always "LI". We could then try to shrink this
set down over time (or not).

-- Sanjoy
>
> Yes it's inconsistent, but IMHO it still conveys so much more
information than the original that the benefit greatly outweighs the cost.
>
> So is the disruption implied by changing to the new convention justified? I
think so.
>
> [1]
https://github.com/llvm/llvm-project/blob/ec0263027811f48b907224ede0dd24c33c1c7507/llvm/lib/Transforms/Vectorize/LoopVectorize.cpp#L7433
> _______________________________________________
> LLVM Developers mailing list
> llvm-dev at lists.llvm.org
> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev

On 2/18/2019 4:15 AM, Michael Platings via llvm-dev
wrote:
> Taking my previous example [1]:
> 
> InnerLoopVectorizer LB(L, PSE, LI, DT, TLI, TTI, AC, ORE, VF.Width, IC,
>    &LVL, &CM);
> 
> If we imagine that over time it evolves such that 50% of the variables have
been renamed to camelBack versions of the type names, then it will look like
this:
> 
> InnerLoopVectorizer LB(loop, PSE, loopInfo, DT, targetLibraryInfo, TTI,
>    assumptionCache, ORE, vectorizationFactor.Width, IC,
>    &loopVectorizationLegality, &CM);
Hold on...

The change from UpperCamel to lowerCamel should be separate from going 
from X to somethingOtherName.

It seems like in this example, TLI is changed to targetLibraryInfo for 
the purpose of having a name that lowerCamel can be applied to, which 
is, at best, backwards.

When a new person sees "TLI", they won't know what it is. When an
LLVM
developer sees "TLI" they know exactly what it is, even without any 
context. At the same time, a person is new to LLVM for only a certain 
period of time, much shorter than the lifetime of the code.

The key to readability is to make the important things easy to see, and 
get the unimportant things out of the way. By using completely expanded 
names we run the risk of making everything equally "easy to see"...

-Krzysztof

On Mon, Feb 18, 2019 at 2:16 AM Michael Platings via llvm-dev <
llvm-dev at lists.llvm.org> wrote:
> Regarding a plan for conversion, I'm keen to avoid perfect being the
enemy
> of better.

It seems a bit early to discuss conversion given there’s not consensus on a
style.  For example:

If we imagine that over time it evolves such that 50% of the variables
have
> been renamed to camelBack versions of the type names, then it will look
> like this:

This comment seems to assume that camelBack is the agreed upon style and
all we need to do now is move forward, and I do not believe that to be the
case
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20190219/059363ca/attachment.html>