Code style is always subjective, but that means that it should only be changed by consensus or at least by clear majority opinion.
Let's just say that 80 cols aren't really being followed in our tree:
$ git grep -E "^.{82}" src/ |wc -l
219683Therefore I reject the idea that there's consensus that our coding style _is_ 80 cols.
So you "reject the idea" that the coding style documentation which *you just changed yourself* in CB:31771 used to clearly state an 80 column limit? Or that we had a big discussion about this last year that was clearly framed as moving from the current standard of 80 columns to something else? Or that the linter has been checking exactly this limit for years? I can also reject the idea that the sun came up today but that doesn't really make it any less true.
Of course not all code in our repo complies to it, that's probably true for almost every single style rule we have. Some reviewers don't pay as much attention as others. Some code is very old, from before we had automatic linting which made it easy for stuff to slip through. And as mentioned, there *are* valid exceptions where it may make sense to go over the limit in individual cases. Yet
$ grep -E "^.{1,79}$" src/ | wc -l
2184454
is an order of magnitude larger, and if you use --include="*.c" you'll find that 3/4ths of the violations are in Makefiles (where the style doesn't apply, at least that's how we've been seeming to hold it) and in headers (where it's mostly big tables and other special cases) anyway.
and completely ignoring the fact that there are hundreds of other developers on this project who may or may not agree with your subjective change. *That* is what I'm opposed against!
Have fun opposing your imaginary straw man with your imaginary developer army, but please leave me out of that.
My "imaginary developer army" voiced its opinion last year when this was actually brought up on the mailing list first as it should, and there was a pretty equal split between for and against. Are you saying that they shouldn't have a say because they're not all monitoring every single change uploaded to docs and util directories? It's not on me to prove that your giant change which affects every single developer has no support in the community. It's on you to prove that it does, and to give opportunity for discussion before you make it.
so you can upload a patch that sets "ColumnLimit: 80" and I'll happily +2 that.
I thought the rough consensus was "80 columns except where it _really_, _really_ makes sense"? In that case ColumnLimit: 80 (which sets a hard boundary) is of no use.
I'm not the one arguing for the use of clang-format here. Our coding style says 80 columns with exceptions. If clang-format cannot model that, then find a way around that, or choose not to use it. A reasonable approach might be to set it to 80 columns and implement it as a linter warning that can be ignored on a case-by-case basis where necessary.
you can't just change it willy-nilly without making some effort to confirm that most of the community is in favor of your change first.
I think this discussion is going on and off for longer than you contribute to coreboot, and it was only recently that most people who participated grudgingly agreed to something (for some, 96 is still _way_ too limiting). "willy-nilly"? Please.
I'm sorry I wasn't around when you and Ron had a private chat about line lengths back in 1999, but for the last 5+ years 80 chars has been lived as a rule in every coreboot CL I've seen. I've been told to follow it by other reviewers, and I've told developers in reviews to follow it. I also don't recall any discussion on the mailing list about this topic in that timeframe besides the one last year. Trying to invoke some vague memory from ancient past doesn't really disprove that it has been done for quite a while and changing it now would be a big departure from that.
And I'm not aware where most people "grudginly agreed" to anything. The mailing list thread petered out with opposite opinions (in fact I already counted them back then: 4 people (excluding myself) in favor of sticking to 80 chars, and 4 people in favor of doing various other things). Then Martin uploaded CB:27533 anyway, I objected that there was no clear majority on this, there was some more arguing with various people for and against, and the issue was abandoned without resolution. I don't see your "grudging agreement" anywhere.
The mere existence of util/lint/lint-*022* disagrees with "just an option" (and note, the only I change I did in that space was to move it from experimental to stable, but util/lint is the wrong place for optional non-rules.)
The existence proves that somebody committed it (likely again without really involving the community), nothing more. I still don't see a .clang-format-scope file anywhere in the tree so I'm not even sure how this test is supposed to do anything at this point. But clang-format is only tangentially related to the question of line length.
(I am not sure what setting ColumnLimit: 0 *and* removing the checkpatch check is supposed to accomplish... if you're trying to say we should remove the line length limit altogether, then no, I'm obviously not in favor of that either.)
ColumnLimit: 0 doesn't mean that clang-format makes every line as long as possible, but that it leaves lines alone that it doesn't touch and otherwise creates lines as long as necessary. A concession to that "80 columns except where it makes sense" thing.
Patch Set 2:
Since apparently the strongest opinions on that (not necessarily a "majority" by any metric) are for artisanal source code, I guess the only resort we have is to drop both checkpatch and clang-format and continue to rely on the services of Paul and Elyes to keep us in line with a style guide that nobody really follows anyway. I'll prepare a patch.I always thought I have the strongest opinion :-P If there is a little consensus, I could try to adapt checkpatch to an indentation + x rule. But maybe we should first decide if we want clang-format.
Since there's dissent on not doing anything, and on doing anything, I'll step back and let you folks who care strongly get to an agreement ('you' means: Julius, Nico, Ron, Philipp and whatever posse you bring).
Try to keep it non-personal though, don't engage in sinister plots and don't accuse others of doing so. Thanks.
In case you guys agree on using clang-format to enforce coding style and a rule set that a computer can reasonably create, I'll see that clang-format will support it properly (count this as a weak "let's use clang-format to enforce style, no matter which"), but other than that I'm out.
Right now I care that the line length is not changed without discussion. That means CB:31771 and the checkpatch part of this CL should be reverted for the time being. I don't care about clang-format for now because I'm not using it. If we start forcing everyone to use it or using it to throw linter errors that may change, but since that currently does not seem to be the case we can cross that bridge when we get there.
If you want to keep the clang-format setting at 0 so you can use your own judgement on line lengths, feel free to do that. But then if you upload a CL with a line over 80 the linter should throw a warning and you can discuss with the reviewers whether this is one of the rare exceptions allowed by our style. The coding style documentation should say 80, and the lived review culture in the project should default to 80 with rare exceptions.
To view, visit change 31651. To unsubscribe, or for help writing mail filters, visit settings.