I’ve noticed that LLVM code tends to use the style of putting short case clauses on a single line, e.g.:
switch (Lex.getKind()) {
default: CC = CallingConv::C; return false;
case lltok::kw_ccc: CC = CallingConv::C; break;
case lltok::kw_fastcc: CC = CallingConv::Fast; break;
case lltok::kw_coldcc: CC = CallingConv::Cold; break;
case lltok::kw_cfguard_checkcc: CC = CallingConv::CFGuard_Check; break;
However, because we have AllowShortCaseLabelsOnASingleLine set to false in our clang-format style, it wants to format it as:
switch (Lex.getKind()) {
default:
CC = CallingConv::C;
return false;
case lltok::kw_ccc:
CC = CallingConv::C;
break;
case lltok::kw_fastcc:
CC = CallingConv::Fast;
break;
case lltok::kw_coldcc:
CC = CallingConv::Cold;
break;
case lltok::kw_cfguard_checkcc:
CC = CallingConv::CFGuard_Check;
break;
Should the LLVM clang-format style be changed to match what seems to be the project’s de-facto style?
Yes, definitely, I was not aware clang-format had this feature, AllowShortCaseLabelsOnASingleLine. I always assumed this was missing functionality, and that nobody would bother implementing our weird bespoke code style.
Honestly, I find that code from the first example horrid and would never write it as such ever. Consequently, I’d much rather keep the style enforced by clang-format.
Do you have any figures of how much code would change in either direction if the whole code-base was reformatted with/without this change? I think it would help to have some hard data to confirm that one style is preferred over the other already in the codebase, because I don’t recall seeing any case statements like that in code I’ve worked on (and if I’d touched such code, I’d probably have reformatted it, since it didn’t match the clang-format style).
If you run it before and after the change, you could do a diff of the diffs )or something equivalent) to get an idea of how the change affects things, right?
I think what might be the data you’re looking for is to clang-format everything with the option, and re-count the instances of this style afterwards.
Restricting to cpp and h files this time ('llvm/**/*.cpp' 'llvm/**/*.h' ':!llvm/test/'), we currently have 3078 instances. After re-clang-formatting all of them with AllowShortCaseLabelsOnASingleLine we then have 6970.
So, a bit less than half the code is already in the one-line style, but half is not.
Looking at clang, instead, the numbers are currently 767, afterwards 2834.
My view is that, prior to the advent of clang-format, the one-line case style was preferred. Post clang-format adoption, clang-format broke the concise, one-line formatting, and we have gradually drifted away. Given that almost half of these switches are still compact suggests that people are applying some intentional effort to preserve them.
IMO the best application of this compact style is with return, so you can do a mapping from enum to value, something like:
int getValueForEnum(Enum e) {
switch (e) {
case Enum::A: return p->a;
case Enum::B: return p->b;
case Enum::C: return p->c;
}
return myDefault();
}
I don’t have the historical context so can’t comment on that. I note that the style isn’t directly referenced in the coding standards documentation, which I expect it should be, regardless of the conclusion of this discussion.
Either way, this thread really needs to be “what should the style be?”
I could maybe be persuaded for the single line case for single statements like the returning of values as described, but even then I’d ask how this xis really any different from simple if statements. For those with more than one statement, as in most of the original example, I’d be against it: why is a case any different from other areas of code?
+1 for allowing switch statements with single-line cases.
It is quite common for switch statements to be fairly regular mappings of simple cases as in the initial example by @jyknight. Expanding them to cover more lines wastes vertical space and makes the code harder to read. The point of keeping those cases on single lines is to expose the regularity of the cases.
This is different from single-line if-statements. Long sequences of short and regular if-statements are extremely rare.
As a philosophical aside, this is yet another example where the clang-format approach of “There is only one way to format code, and it must be easily decidable by a machine” falls short. It is very common that single-line cases make simple, regular switch statements clearer. However, I also would expect to see cases where clang-format will single-mindedly decide that one case of a larger switch statement fits in a single line and put it there even when doing so is nonsense from a higher-level perspective of the code. If I had my way, I would allow both ways of writing a switch statement, with the restriction that a consistent style must be used within each switch statement, and have clang-format preserve the style.
I’m (still) +1 on single line cases. I think it helps readability significantly, e.g. in the case that @rnk pointed out upthread. I also didn’t know clang-format supported this!
+1 for enabling AllowShortCaseLabelsOnASingleLine. The tabular view does generally look conciser and more readable, and in the mapping case very valuable.
It’d be great if AllowShortCaseLabelsOnASingleLine actually meant “allow” and not “force”, i.e. that’d it’d just leave the formatting of the short cases to the programmer, but I guess clang-format doesn’t work like that.
This. I’m happy to accept that there are some valid cases for this style, but I don’t want multiple short statements to be folded onto the same line just because we’re in a case.
Relatedly, a developer is currently trying to reformat the llvm-readobj code, and clang-format is modifying some switch statements in this format. Given this discussion, I’m not inclined to let that bit of the change through, but preventing clang-format touching that at all seems too heavyweight. Any suggestions on how to proceed? (⚙ D122840 [llvm-readobj][nfc] Run clang-format on the directory's .cpp and .h files)
Yes, that’s what I’m referring to: this prevents all formatting of the region, but there may be aspects of it that want reformatting (e.g. indentation etc).
I’m late here, but I vote against allowing this. What we will end up with in some cases is a mixture of single-line and multi-line cases, depending on whether clang-format will be able to squeeze the contents into a single line. Also, the first example in this thread does not look very readable to me after being formatted:
switch (Lex.getKind()) {
default: CC = CallingConv::C; return false;
case lltok::kw_ccc: CC = CallingConv::C; break;
case lltok::kw_fastcc: CC = CallingConv::Fast; break;
case lltok::kw_coldcc: CC = CallingConv::Cold; break;
case lltok::kw_cfguard_checkcc: CC = CallingConv::CFGuard_Check; break;
}
Multiple statements won’t get folded, in general – it limits the number of statements to only 1 or 2, and no nested if/while/for/switch. That typically means you would have a single statement followed by a break, or a single return. So, e.g. case 4: AA=1; BB=2; break; would not get folded to one line, despite being a small number of characters.
Although, in odd corner-case code with fallthrough, that rule could result in unexpected output. E.g. case X: AA=1; BB=2 with no break, would get folded. That seems unexpected – it’d probably be better for it to require break, 1 other statement followed by break, or return.
This example got me thinking. Could clang-format be persuaded to do the following style?
switch (Lex.getKind()) {
default: CC = CallingConv::C; return false;
case lltok::kw_ccc: CC = CallingConv::C; break;
case lltok::kw_fastcc: CC = CallingConv::Fast; break;
case lltok::kw_coldcc: CC = CallingConv::Cold; break;
case lltok::kw_cfguard_checkcc: CC = CallingConv::CFGuard_Check; break;
}
This doesn’t look so bad, because it is much more regular. I’d also agree that if you end up with some cases that don’t fit on one line, then none of them should in the above style.