@John_McCall mentioned that he was surprised that I was using lower camel case for fields. I thought it would be useful to reach consensus with people who work in SIL/SILOptimizer what style we want/document this.
Previously in SIL/SILOptimizer we were following an old school LLVM style that had locals/fields/types all upper camel case. I.e.:
class MyClassType {
MyOtherType Foo;
void doSomething() {
auto LocalFoo = Foo;
}
};
Recently, we have been trying to move the SILOptimizer more to a style (that I believed to be anyways) more like SILGen. This implied using lower camel for fields and locals, but keeping types as being upper camel.
I am fine if we want to use this style. Lets just all agree so we can all together incrementally update.
I don't think we should have a different style for SIL and SILOptimizer; we should use the same convention throughout the compiler. Our criteria should be (1) something we're all happy enough with and (2) something that we can be consistent about.
We could just use standard Swift style in the C++ code so that the coding style consistent across the entire repository regardless of source language. I'd be happy enough with that. I do personally prefer to mark fields differently, which Swift style doesn't, but I don't care enough to hold on to that.
I don't think we should continue using the LLVM style in Swift. I don't think anyone actually likes it, so it doesn't meet one of the two criteria above.
Is there a reformatting tool we can just run? I think we're pretty much done with the 4.2 branch, so this isn't the worst time to do a reformat.
I agree, whatever style is decided on should be consistent in the compiler and documented somewhere so that new contributors don’t have to try and understand how they should format code.
SGTM. I also think it is important to document it in ./docs so that people (e.g. me) have something to reference if they forget.
I am fine with marking fields differently in an abstract sense. My issue with using upper camel is that then field names clash with type names. That being said, as long as what we have is documented, I am fine with whatever direction people want to go in.
I agree. I hate the clash in between types and variable names.
I think we could do some sort of clang-format based thing. Are you proposing a large-scale reformatting? We could also try to be incremental about this once we have something that is documented and clear.
The most important thing for me here is the documentation. This style comes up every once in a while and I would like to never have to think about it again if possible ; ).
Ok. I am willing to drive that assuming there aren't objections. I am going to wait a bit to see if we get more feedback on this thread and then create a separate proposal thread.
I think this is a pretty solid, objective argument in an area where most arguments are squishy and subjective. It would help contributors coming from the Swift side adapt to the C++ parts of the compiler, while also helping existing contributors when they need to touch something in the language they don’t usually work in.
(On the other hand, it would make moving between LLVM/clang and Swift a bit more cumbersome. How much do we do that?)
Most of our contributors don't really do it at all, but there are exceptions — Adrian Prantl, Arnold Schwaighofer, and myself among others. Speaking only for myself, I'd adapt.
FWIW, I consider it a long term goal for local variables to all move to lowercase in LLVM and as major refactorings of subsystems happen, this is slowing happening. I also would love to see this happen for Swift.
IMO, the questions comes down to "what is the threshold for a 'major refactoring'", and "does Swift want to align with the rest of the LLVM community or be intentionally different"?
Also, I think is worth distinguishing between the "old school" style and the "desired style". The desired LLVM style is "capitalized types" and "lowercase values" (functions, variables etc). This is very similar to Swift code style.
... or perhaps I just completely mis-remember the discussion. I remember the discussion, but it doesn't look like it converged, and has now led to annoying stuff like this because the existing rule doesn't make any sense.
I am +1 for moving all of llvm to lower case names for variables, but that will itself be a big community discussion........
Okay. It sounds like we are all agreed on camelCase names for functions and UpperCase names for types. Points of possible discussion:
lowerCase for variables or lower_case? (I'd prefer the former.)
Any sort of decoration for instance variables? (I'd faintly prefer a trailing underscore.)
SGF or sgf for initialism names of variables? (I'd prefer the latter for consistency.)
UpperCase for enumerators, or should we follow the Swift convention of camelCase? Does being a scoped enumerator affect this? (I would argue for UpperCase and say no.)
Should function-like values be given verb phrases for names? (I'd say yes, although I'm torn about simple things like predicate.)
lowerCase for variables or lower_case ? (I'd prefer the former.)
I strongly prefer the former.
Any sort of decoration for instance variables? (I'd faintly prefer a trailing underscore.)
I weakly prefer "no" for consistency with LLVM, and because the need for this is also a pretty significant code/design smell.
SGF or sgf for initialism names of variables? (I'd prefer the latter for consistency.)
There is almost always a better name to use, than repeating the type name. What role does SGF provide? functionContext?
UpperCase for enumerators, or should we follow the Swift convention of camelCase ? Does being a scoped enumerator affect this? (I would argue for UpperCase and say no.)
I weakly prefer Upper case.
Should function-like values be given verb phrases for names? (I'd say yes, although I'm torn about simple things like predicate .)
The first is the "singleton" manager classes like SILGenFunction. It is better to just accept the use of idiomatic initialisms for these references than to encourage the use of "meaningful" names like "functionEmitter" which are both verbose and likely to arbitrarily vary in different scopes. (And in particular we should strive to avoid introducing yet another thing called "context", which is quite badly overused in the compiler already.)
The second is naming parameters in e.g. visitor methods. Giving these throwaway names is fine.
Anyway, I don't think the code-style document can get away with just saying that you should never use an initialism. It's okay to say that initialisms don't get a special-case exception to the capitalization rules, though.
I have no opinion on what to call these things. Using all lower case or all upper case is fine. Please be consistent though one way or the other. The number of different ways to spell getContext() throughout the compiler is maddening. :-)
So I think we have a consensus no? I can write up something over the week as a coding standards document (that defers to LLVM, but makes this style clear).
I'm not sure it makes sense to "defer to LLVM"; these rules aren't based on the actual LLVM style guide, and we're not a subproject of LLVM. If you want to cite LLVM as a source/inspiration, that sounds great.