Patrick Georgi has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/31651
Change subject: lint/clang-format: set to 96 chars per line ......................................................................
lint/clang-format: set to 96 chars per line
80 chars + 2 tabs was the compromise we got to in the last round of discussion.
Change-Id: I9293a69d1bea900da36501cde512004d0695ad37 Signed-off-by: Patrick Georgi pgeorgi@google.com --- M .clang-format M util/lint/lint-007-checkpatch 2 files changed, 5 insertions(+), 3 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/51/31651/1
diff --git a/.clang-format b/.clang-format index d853f50..5c8aa3c 100644 --- a/.clang-format +++ b/.clang-format @@ -7,7 +7,7 @@ IndentCaseLabels: false SortIncludes: false ContinuationIndentWidth: 8 -ColumnLimit: 0 +ColumnLimit: 96 AlwaysBreakBeforeMultilineStrings: true AllowShortLoopsOnASingleLine: false AllowShortFunctionsOnASingleLine: false diff --git a/util/lint/lint-007-checkpatch b/util/lint/lint-007-checkpatch index afa593e..a7b63e8 100755 --- a/util/lint/lint-007-checkpatch +++ b/util/lint/lint-007-checkpatch @@ -28,6 +28,8 @@ ^src/vendorcode|\ ^Documentation"
+opts="--max-line-length 96" + # default: test src and util if [ "$1" = "" ]; then INCLUDED_DIRS="src util" @@ -35,7 +37,7 @@ elif [ "$1" = "diff" ]; then args=$( echo $EXCLUDED_DIRS | \ sed -e 's,\|, ,g' -e 's,^,--exclude=,g' ) - util/lint/checkpatch.pl --quiet --no-signoff $args - + util/lint/checkpatch.pl --quiet --no-signoff $opts $args - exit $? # Space separated list of directories to test else @@ -49,5 +51,5 @@ grep -v $EXCLUDED_DIRS )
for FILE in $FILELIST; do - util/lint/checkpatch.pl --show-types --file --quiet "$FILE" + util/lint/checkpatch.pl --show-types --file --quiet $opts "$FILE" done
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31651 )
Change subject: lint/clang-format: set to 96 chars per line ......................................................................
Patch Set 1: Code-Review+1
Do we want to make this a recommendation? Then we should also update `Documentation/coding_style.md`.
Martin Roth has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31651 )
Change subject: lint/clang-format: set to 96 chars per line ......................................................................
Patch Set 1: Code-Review+2
Do we want to make this a recommendation? Then we should also update `Documentation/coding_style.md`.
I think we still want to recommend that lines stay under 80 characters when it makes sense, but instead of it being a hard limit, it's just a recommendation. When it makes the code harder to read at 80 characters, the coding style allows for lines up to 96 characters.
I do agree that we should update the coding style document.
Martin Roth has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31651 )
Change subject: lint/clang-format: set to 96 chars per line ......................................................................
Patch Set 1:
I guess my earlier comment doesn't make sense if we're going to automatically format the code.
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31651 )
Change subject: lint/clang-format: set to 96 chars per line ......................................................................
Patch Set 1:
(1 comment)
In my opinion, the configuration of clang-format still needs to be tweaked to follow checkpatch in several things.
https://review.coreboot.org/#/c/31651/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/31651/1//COMMIT_MSG@10 PS1, Line 10: discussion. Please add the URL for the discussion, so that people can easily read the arguments.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31651 )
Change subject: lint/clang-format: set to 96 chars per line ......................................................................
Patch Set 1: Code-Review+2
(1 comment)
In my opinion, the configuration of clang-format still needs to be tweaked to follow checkpatch in several things.
AIUI, we're not going to enforce it on all the code at once. Let's see where it gets us. Further tuning to the config wouldn't be part of this change anyway.
https://review.coreboot.org/#/c/31651/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/31651/1//COMMIT_MSG@10 PS1, Line 10: discussion.
Please add the URL for the discussion, so that people can easily read the arguments.
I'm not sure if it was a recorded discussion ;)
Nico Huber has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/31651 )
Change subject: lint/clang-format: set to 96 chars per line ......................................................................
lint/clang-format: set to 96 chars per line
80 chars + 2 tabs was the compromise we got to in the last round of discussion.
Change-Id: I9293a69d1bea900da36501cde512004d0695ad37 Signed-off-by: Patrick Georgi pgeorgi@google.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/31651 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Martin Roth martinroth@google.com Reviewed-by: Nico Huber nico.h@gmx.de --- M .clang-format M util/lint/lint-007-checkpatch 2 files changed, 5 insertions(+), 3 deletions(-)
Approvals: build bot (Jenkins): Verified Martin Roth: Looks good to me, approved Nico Huber: Looks good to me, approved
diff --git a/.clang-format b/.clang-format index d853f50..5c8aa3c 100644 --- a/.clang-format +++ b/.clang-format @@ -7,7 +7,7 @@ IndentCaseLabels: false SortIncludes: false ContinuationIndentWidth: 8 -ColumnLimit: 0 +ColumnLimit: 96 AlwaysBreakBeforeMultilineStrings: true AllowShortLoopsOnASingleLine: false AllowShortFunctionsOnASingleLine: false diff --git a/util/lint/lint-007-checkpatch b/util/lint/lint-007-checkpatch index afa593e..a7b63e8 100755 --- a/util/lint/lint-007-checkpatch +++ b/util/lint/lint-007-checkpatch @@ -28,6 +28,8 @@ ^src/vendorcode|\ ^Documentation"
+opts="--max-line-length 96" + # default: test src and util if [ "$1" = "" ]; then INCLUDED_DIRS="src util" @@ -35,7 +37,7 @@ elif [ "$1" = "diff" ]; then args=$( echo $EXCLUDED_DIRS | \ sed -e 's,\|, ,g' -e 's,^,--exclude=,g' ) - util/lint/checkpatch.pl --quiet --no-signoff $args - + util/lint/checkpatch.pl --quiet --no-signoff $opts $args - exit $? # Space separated list of directories to test else @@ -49,5 +51,5 @@ grep -v $EXCLUDED_DIRS )
for FILE in $FILELIST; do - util/lint/checkpatch.pl --show-types --file --quiet "$FILE" + util/lint/checkpatch.pl --show-types --file --quiet $opts "$FILE" done
Patrick Georgi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31651 )
Change subject: lint/clang-format: set to 96 chars per line ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/#/c/31651/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/31651/1//COMMIT_MSG@10 PS1, Line 10: discussion.
I'm not sure if it was a recorded discussion ;)
The discussion was recorded on the mailing list (see https://mail.coreboot.org/hyperkitty/search?mlist=coreboot%40coreboot.org&am...), but I don't think the decision was.
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31651 )
Change subject: lint/clang-format: set to 96 chars per line ......................................................................
Patch Set 2:
(1 comment)
This is something that many people feel very strongly about and we never really agreed on. Feel free to reopen the discussion if you want, but can we please revert this patch in the meantime?
https://review.coreboot.org/#/c/31651/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/31651/1//COMMIT_MSG@10 PS1, Line 10: discussion.
The discussion was recorded on the mailing list (see https://mail.coreboot. […]
As far as I am aware there was no conclusive decision on this topic. After the thread you linked, the discussion continued in https://review.coreboot.org/c/coreboot/+/27533 and did not reach a conclusion. Note the heated debate and the strong feelings about this in that discussion. Let's please not try to silently sneak in a change that many people are strongly against and that has not been proven to carry a majority in the community.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31651 )
Change subject: lint/clang-format: set to 96 chars per line ......................................................................
Patch Set 2:
This is something that many people feel very strongly about and we never really agreed on. Feel free to reopen the discussion if you want, but can we please revert this patch in the meantime?
I would prefer not to revert. No matter the reasoning and latest discussions, this brings the configuration closer to our coding style (that was copied from the Linux kernel years ago). It says you may exceed the 80 chars limit if it increases readability.
So if the 80 chars aren't a hard limit, I don't see why we should let tools bother us with it.
Patrick Georgi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31651 )
Change subject: lint/clang-format: set to 96 chars per line ......................................................................
Patch Set 2:
Patch Set 2:
(1 comment)
This is something that many people feel very strongly about and we never really agreed on. Feel free to reopen the discussion if you want, but can we please revert this patch in the meantime?
Indeed, some people feel very strongly that 96 (or less, the horror!) is way too restrictive. That's what compromises are for: nobody is entirely happy, but everybody can somehow live with them.
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31651 )
Change subject: lint/clang-format: set to 96 chars per line ......................................................................
Patch Set 2:
I would prefer not to revert. No matter the reasoning and latest discussions, this brings the configuration closer to our coding style (that was copied from the Linux kernel years ago). It says you may exceed the 80 chars limit if it increases readability.
Come on, that's not what that means and you know it. If you try to submit a patch to the Linux kernel where a line goes to 96 characters without good justification they'll reject you, as they always have. If you want to change the linter warning to "please strongly reconsider whether this line needs to be that long" or something like that, fine... but this change is currently being used as justification to rewrite lines in existing files that have absolutely no business being longer than 80 chars (e.g. https://review.coreboot.org/c/coreboot/+/31816/5/src/security/tpm/tspi.h), and that was clearly not the original intention of that coding style. You can't just reinterpret something that we have always interpreted for years as a pretty hard limit outside of really special cases and say "oh, 'increases readability' is clearly always given until you reach 96 chars".
Indeed, some people feel very strongly that 96 (or less, the horror!) is way too restrictive. That's what compromises are for: nobody is entirely happy, but everybody can somehow live with them.
This was already discussed at length previously (e.g. in CB:27533). The arguments for 80 characters are not primarily for the number itself but for consistency with other projects. Moving it to 96 characters isn't some compromise that makes everyone somewhat happy, it's just as bad for consistency as any other limit that's not 80.
I don't think it's appropriate to just sneak in such a wide-ranging and clearly very controversial policy change in the night without letting anyone know, especially when you previously tried and failed to convince the community at large of it. If I found someone else who's willing to +2 a revert of this and snuck that in without you noticing, would you be okay with that? Do we have to escalate this into a force submit war until we eventually try to remove each others' commit access or something like that? We have a community with many active members, we have a mailing list to discuss things and find a consensus, and when that doesn't work there are other ways to at least make a majority decision (e.g. that vote that was discussed but never implemented last time), but this is clearly not one of them. Whenever I try to make a change that's even remotely affecting the community at large I try my best to let potentially interested people know and give a heads-up and room for discussion on the mailing list, and it honestly pisses me off when others just secretly change the world overnight and then smugly sit on their fait accompli.
This change is very controversial and it's not clear that the majority of the community supports it. I'd like you to revert it until you can show that it does.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31651 )
Change subject: lint/clang-format: set to 96 chars per line ......................................................................
Patch Set 2:
Wow, I didn't expect that we'd let blunt clang-format patches in. Now I understand what the fuss is about.
If people want to dully use a formating tool (which probably can never decide if obeying a soft limit is reasonable), that tool should be set to 80 chars. So, yeah, I guess we should revert the clang-format part at least.
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31651 )
Change subject: lint/clang-format: set to 96 chars per line ......................................................................
Patch Set 2:
I think there's some misunderstanding about what happened here, Nico... sounds like you think we just loosened the linter a bit for those truly exceptional cases the coding style allows, but I think everyone else (e.g. Patrick) thinks the new default limit for everything is now 96. That's what I'm opposed to. (Note that CB:31771 also just changed the coding style documentation.)
I am not saying that you may never ignore the linter if you have a truly exceptional case where everyone agrees not breaking lines helps readability. Many of our linter warnings have occasional false positives that we intentionally ignore anyway, and that's fine. As I said, if you want to clarify the warning text to point out the possibility of rare exceptions, I think that would be okay. (I do thing we should have some sort of warning for the default limit, though, just because it helps a lot with catching mistakes.)
But I am strongly opposed to changing the default limit and I think such a sweeping and controversial change should absolutely not be made the way it was made here.
Patrick Georgi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31651 )
Change subject: lint/clang-format: set to 96 chars per line ......................................................................
Patch Set 2:
Patch Set 2: This was already discussed at length previously (e.g. in CB:27533). The arguments for 80 characters are not primarily for the number itself but for consistency with other projects. Moving it to 96 characters isn't some compromise that makes everyone somewhat happy, it's just as bad for consistency as any other limit that's not 80.
These "other projects" would be mostly Linux, since that's the style we're closest to: try getting our code into a GNU project (for example) without reformatting it entirely.
But we are not Linux. IS_ENABLED() has different semantics than Linux's, oh, and we use CONFIG() now. Our Kconfig has different semantics than Linux's. We use volatile, which is frowned upon in Linux.
I don't think it's appropriate to just sneak in such a wide-ranging and clearly very controversial policy change in the night without letting anyone know,
I wasn't aware that it was night from Feb 27 to Mar 5.
This commit doesn't change everything from 80 to 96: it changes checkpatch from 80 to 96 and clang-format from "whatever" to 96. A plain revert would bring that inconsistency back and so I'm strongly opposed to that.
But how about this: We modify checkpatch so we can disable its coding style checks (as opposed its more useful tests about CONFIG(), for example) and do that for everything that is covered by clang-format (as defined in .clang-format-scope).
That also neatly solves the remaining consistency issues (see the checkpatch warnings in CB:31652 that complain about code as produced by clang-format) by having one tool decide on style instead of two tools fighting it out.
With that we can go back to clang-format's ColumnLimit: 0 (ie its "whatever" option) for the code it handles and checkpatch back to 80 for the code that it doesn't.
ron minnich has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31651 )
Change subject: lint/clang-format: set to 96 chars per line ......................................................................
Patch Set 2:
experiences from Go:
1. "nobody likes the output of gofmt; everyone likes gofmt" not everyone likes every single thing about gofmt, but everyone likes not having to worry about formatting.
2. The arguments over requiring gofmt were about as angry as the arguments here; they're all forgotten now
3. gofmt output has changed over time. So, should we decide in 20 years that 102 char lines are ok, we can.
I think Patrick's idea makes a lot of sense.
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31651 )
Change subject: lint/clang-format: set to 96 chars per line ......................................................................
Patch Set 2:
These "other projects" would be mostly Linux, since that's the style we're closest to: try getting our code into a GNU project (for example) without reformatting it entirely.
But we are not Linux. IS_ENABLED() has different semantics than Linux's, oh, and we use CONFIG() now. Our Kconfig has different semantics than Linux's. We use volatile, which is frowned upon in Linux.
This was just part of the argument why some people prefer 80 characters, and why 96 is not "a compromise". No, we are not Linux, and we don't have to be, but many people working on this project come from Linux (or U-Boot or any of the other dozens of projects that copy its style) and many of them prefer to have more consistent style between the different projects they work on and sometimes port code back and forth between.
I am not saying that we have to be Linux. I am saying that you're making a sweeping change to the project just because you felt like it without taking any of the objections that you knew were there into account or even trying to discuss the issue again first. Code style is always subjective, but that means that it should only be changed by consensus or at least by clear majority opinion. You don't have that here, you're just doing whatever you want 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!
This commit doesn't change everything from 80 to 96: it changes checkpatch from 80 to 96 and clang-format from "whatever" to 96. A plain revert would bring that inconsistency back and so I'm strongly opposed to that.
Uhh... sorry, what? You yourself just submitted CB:31771 which literally says in the subject "our coding style now allows 80 + 2*8 columns in a line", citing this patch as justification. I am arguing against and asking you to revert both of these, obviously.
If you want to align clang-format to the coding style we use in coreboot, the status quo on that clearly is 80, so you can upload a patch that sets "ColumnLimit: 80" and I'll happily +2 that. If you instead want to change the line length of our coding style, please be respectful of the fact that this is a very controversial topic that affects every single developer on the project, many of whom have an opinion about it, and 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.
But how about this: We modify checkpatch so we can disable its coding style checks (as opposed its more useful tests about CONFIG(), for example) and do that for everything that is covered by clang-format (as defined in .clang-format-scope).
That also neatly solves the remaining consistency issues (see the checkpatch warnings in CB:31652 that complain about code as produced by clang-format) by having one tool decide on style instead of two tools fighting it out.
With that we can go back to clang-format's ColumnLimit: 0 (ie its "whatever" option) for the code it handles and checkpatch back to 80 for the code that it doesn't.
As far as I am aware we consider clang-format an option for developers who chose to use it, and there are no plans to change that. I would likely be opposed to it if there were (and I assume others would be as well, e.g. it sounds like Nico is not a fan either). But that is a separate topic from the line-length discussion, and as mentioned above, I am perfectly fine with matching the line length limit we have in .clang-format if that's what you want to do. (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.)
Patrick Georgi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31651 )
Change subject: lint/clang-format: set to 96 chars per line ......................................................................
Patch Set 2:
Patch Set 2: just because you felt like it without taking any of the objections that you knew were there into account
I did take them into account. At some point everything boils down to a small set of fundamentally incompatible objections to different options. It's called a compromise.
or even trying to discuss the issue again first.
"Discuss as much as you please as long as in the end you do what I want"?
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 219683
Therefore I reject the idea that there's consensus that our coding style _is_ 80 cols.
You don't have that here, you're just doing whatever you want
Note that I didn't merge this change (and I would have let it linger for a while longer, specifically to sort out the remaining questions). I'm no stranger to having commits hang around for 2 years or longer.
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.
If you want to align clang-format to the coding style we use in coreboot,
Which coding style would that be, in practice?
the status quo on that clearly is 80,
Clearly not. See above.
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.
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.
As far as I am aware we consider clang-format an option for developers who chose to use it, and there are no plans to change that.
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.)
I would likely be opposed to it if there were (and I assume others would be as well, e.g. it sounds like Nico is not a fan either).
As Ron reports from other projects, nobody is happy with it before, while everybody seems to accept it afterwards.
(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.
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.
Philipp Deppenwiese has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31651 )
Change subject: lint/clang-format: set to 96 chars per line ......................................................................
Patch Set 2:
Patch Set 2:
Patch Set 2: just because you felt like it without taking any of the objections that you knew were there into account
I did take them into account. At some point everything boils down to a small set of fundamentally incompatible objections to different options. It's called a compromise.
or even trying to discuss the issue again first.
"Discuss as much as you please as long as in the end you do what I want"?
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 219683
Therefore I reject the idea that there's consensus that our coding style _is_ 80 cols.
You don't have that here, you're just doing whatever you want
Note that I didn't merge this change (and I would have let it linger for a while longer, specifically to sort out the remaining questions). I'm no stranger to having commits hang around for 2 years or longer.
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.
If you want to align clang-format to the coding style we use in coreboot,
Which coding style would that be, in practice?
the status quo on that clearly is 80,
Clearly not. See above.
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.
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.
As far as I am aware we consider clang-format an option for developers who chose to use it, and there are no plans to change that.
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.)
I would likely be opposed to it if there were (and I assume others would be as well, e.g. it sounds like Nico is not a fan either).
As Ron reports from other projects, nobody is happy with it before, while everybody seems to accept it afterwards.
(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.
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 think the whole discussion regarding the line length as a waste of time. Who cares about that, modern software development comes with an auto linting mechanism. We don't want to rely on human resources linting our code manually. I guess Werner will agree including the folks from Facebook as well. So, please keep checkpatch and improve the clang-format and make it default linting every code going upstream. I don't want to do legacy software development anymore.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31651 )
Change subject: lint/clang-format: set to 96 chars per line ......................................................................
Patch Set 2:
Will try to keep this short.
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.
The problem I see with clang-format is that it only knows a simple hard limit. What does a rule like "2 tabs + 80" mean for a tool like clang-format? I couldn't decide. OTOH, it seems easy to adapt checkpatch for such a rule.
As far as I am aware we consider clang-format an option for developers who chose to use it, and there are no plans to change that.
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.)
I have to admit, I don't understand our current setup. Some scripts seem to assume that you opt in for clang- format others don't?
I would likely be opposed to it if there were (and I assume others would be as well, e.g. it sounds like Nico is not a fan either).
As Ron reports from other projects, nobody is happy with it before, while everybody seems to accept it afterwards.
What everybody accepts? Clearly in times of political apathy, the ruler wins. But what does that have to do with this discussion?
My biggest fear is that our review quality could decline. It's not measurable anyway. But to me it seems that longer lines are more eye-weary. In case we have a poll on the matter, I'd love to see an estimate how much time the voter spends on reading code.
(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.
That actually doesn't sound bad.
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.
Patrick Georgi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31651 )
Change subject: lint/clang-format: set to 96 chars per line ......................................................................
Patch Set 2:
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.
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31651 )
Change subject: lint/clang-format: set to 96 chars per line ......................................................................
Patch Set 2:
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 219683
Therefore 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.
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31651 )
Change subject: lint/clang-format: set to 96 chars per line ......................................................................
Patch Set 2:
So before this discussion peters out because nobody wants to respond anymore, I will revert this change (and the Documentation/coding_style.md one). We cannot argue for weeks whether this was/is/should be the right thing to do or not while it already affects all submissions going in. The justification for this in the commit message was "80 chars + 2 tabs was the compromise we got in the last round of discussion", which is simply not true. The policy for bad changes is revert first, sort it out later.
Reverts at CB:31915 and CB:31916
I would strongly suggest to continue this discussion on the mailing list so that all coreboot developers have a chance to join in. I would also suggest to separate the issues of line length and using/forcing clang-format, because both are controversial and it gets very confusing when the discussion keeps going back and forth across both.
-
Julius Werner (Code Review) -
Martin Roth (Code Review)
-
Nico Huber (Code Review)
-
Patrick Georgi (Code Review)
-
Paul Menzel (Code Review)
-
Philipp Deppenwiese (Code Review)
-
ron minnich (Code Review)