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
Chandler Carruth via llvm-dev
2019-Feb-19 02:08 UTC
[llvm-dev] RFC: changing variable naming rules in LLVM codebase
On Mon, Feb 18, 2019 at 10:10 AM Krzysztof Parzyszek via llvm-dev < llvm-dev at lists.llvm.org> wrote:> 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. >FWIW, I suspect separating the transition of our acronyms from the transition of identifiers with non-acronym words may be an effective way to chip away at the transition cost... Definitely an area that people who really care about this should look at carefully.> > 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"... >I think this bias towards acronyms (which I used to share) due to keeping things short but still recognizable once people become deeply familiar with LLVM is the wrong prioritization. It does work well for experienced LLVM developers, but I think we should do much more to facilitate and encourage people who are not in this set. While this does come at some cost to highly experienced LLVM developers (reading `library_info` instead of `TLI`), but it seems easily worth it to make the codebase more accessible to new contributors. -------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20190218/8d82b5ef/attachment.html>
Diana Picus via llvm-dev
2019-Feb-19 10:12 UTC
[llvm-dev] RFC: changing variable naming rules in LLVM codebase
On Tue, 19 Feb 2019 at 03:08, Chandler Carruth via llvm-dev <llvm-dev at lists.llvm.org> wrote:> > On Mon, Feb 18, 2019 at 10:10 AM Krzysztof Parzyszek via llvm-dev <llvm-dev at lists.llvm.org> wrote: >> >> 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. > > > FWIW, I suspect separating the transition of our acronyms from the transition of identifiers with non-acronym words may be an effective way to chip away at the transition cost... Definitely an area that people who really care about this should look at carefully. > >> >> >> 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.I wouldn't downplay context, my first thought when I see TLI is TargetLoweringInfo (of course, then I see Vectorizer around and get back on track, but when it's spelled out that reflex just doesn't kick in to begin with).>> 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.True, but their impression of LLVM when they are new to it may influence their decision to stick with it or not.>> 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"... > > > I think this bias towards acronyms (which I used to share) due to keeping things short but still recognizable once people become deeply familiar with LLVM is the wrong prioritization. It does work well for experienced LLVM developers, but I think we should do much more to facilitate and encourage people who are not in this set. While this does come at some cost to highly experienced LLVM developers (reading `library_info` instead of `TLI`), but it seems easily worth it to make the codebase more accessible to new contributors.+1 for making things accessible to new contributors. Note that this also works for people that aren't new to LLVM per se, but may be new to a given part of the codebase. I'm also not sure it comes at such a high cost to experienced developers, since you'd get used to seeing 'library_info' after a while. It's not like your brain processes every single letter to recognize a word. Personally, when I look through code I decode the acronyms to the full name in my head anyway (it's not something I do on purpose, it's just how it works for me). Just my 2c. Diana> _______________________________________________ > LLVM Developers mailing list > llvm-dev at lists.llvm.org > https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
Michael Platings via llvm-dev
2019-Feb-19 11:27 UTC
[llvm-dev] RFC: changing variable naming rules in LLVM codebase
> FWIW, I suspect separating the transition of our acronyms from the transition > of identifiers with non-acronym words may be an effective way to chip away at > the transition cost... Definitely an area that people who really care about > this should look at carefully.If it makes for an easier transition then I'd be happy to go with upper case acronyms and camelBack for non-acronyms. I've updated https://reviews.llvm.org/D57896 accordingly. -Michael
Chris Lattner via llvm-dev
2019-Feb-20 16:50 UTC
[llvm-dev] RFC: changing variable naming rules in LLVM codebase
> On Feb 18, 2019, at 12:09 PM, Krzysztof Parzyszek via llvm-dev <llvm-dev at lists.llvm.org> wrote: > > 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.I completely agree. I don’t see why “tli” would be problematic. I don’t think that “targetLibraryInfo” would make the code easier to read, write, or maintain. -Chris> > 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 > _______________________________________________ > LLVM Developers mailing list > llvm-dev at lists.llvm.org > https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev