Alex Bradbury via llvm-dev
2019-Feb-15 10:15 UTC
[llvm-dev] 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
Michael Platings via llvm-dev
2019-Feb-18 10:15 UTC
[llvm-dev] RFC: changing variable naming rules in LLVM codebase
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
Sanjoy Das via llvm-dev
2019-Feb-18 19:21 UTC
[llvm-dev] RFC: changing variable naming rules in LLVM codebase
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
Krzysztof Parzyszek via llvm-dev
2019-Feb-18 20:09 UTC
[llvm-dev] RFC: changing variable naming rules in LLVM codebase
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
Zachary Turner via llvm-dev
2019-Feb-19 15:24 UTC
[llvm-dev] RFC: changing variable naming rules in LLVM codebase
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>