Hey folks,
I overeagerly reviewed and submitted a change[1] lately, that set the column limit for our C code to 96. My reasoning was that we already live a "soft" limit of 80 chars and that tools shouldn't complain about every single 8x-chars line (personally, I find this quite annoying during review). What I missed is that people use the limit to format code automatically, resulting in less line breaks, even if they'd make sense for readability.
I don't want to start a discussion about line length here! If we are going to use tools for automatic formatting, we will be limited by the tool's capabilities. So it doesn't make any sense to discuss rules before we know if our tools will allow us to follow them.
So what we should discuss first: if we want to let tools format our code for us. I'll just note some questions/ideas for now. Will try to have an opinion on them later. They are not all mutually exclusive.
Do we want to enforce a single editor / IDE + configuration for coreboot contributions? For instance, Vim is quite configurable and helpful when writing code. This could make all tools for later processing unneces- sary.
Do we want to enforce a single tool, e.g. clang-format, that does the job for us after editing a source file?
The above, even if that implies a new coding style that we might not be used to?
Do we want a combination of such a tool and check-patch? For instance, clang-format has a feature to ignore broken lines. This could then be handled by check-patch, to allow more complex rules.
Do we just want to keep check-patch and let authors / their editors format the code?
Do we want to rely (solely) on reviewers for format checking?
Do we want to encourage reviewers to educate their fellows on code style (for instance, wrt. line length, less indentation levels, shorter, more meaningful identifiers, etc.)?
Nico
Hello,
On Sat, Mar 16, 2019 at 4:32 PM Nico Huber nico.h@gmx.de wrote:
Hey folks,
I overeagerly reviewed and submitted a change[1] lately, that set the column limit for our C code to 96. My reasoning was that we already live a "soft" limit of 80 chars and that tools shouldn't complain about every single 8x-chars line (personally, I find this quite annoying during review). What I missed is that people use the limit to format code automatically, resulting in less line breaks, even if they'd make sense for readability.
I find tools complaining on 8x-char lines rather annoying as well. Especially when a bunch of lines get a warning each, and gerrit's web interface slows down to a complete halt.
Do we want to enforce a single editor / IDE + configuration for coreboot contributions? For instance, Vim is quite configurable and helpful when writing code. This could make all tools for later processing unneces- sary.
IMHO this will cause more trouble than it will solve. I use a mixture of nvim and Geany so I wouldn't mind that much, but other people may use a completely different editor/IDE and I guess they would be rather reluctant to change just to contribute to coreboot.
Do we want to enforce a single tool, e.g. clang-format, that does the job for us after editing a source file?
The above, even if that implies a new coding style that we might not be used to?
One thing that I *really* hate w.r.t. tools is when they want to do something in a specific way and don't allow manual changes. IMHO this doesn't sound like a good idea.
However, I understand that manually maintaining everything in shape is a daunting task, and why some people would want an automated tool.
Do we want a combination of such a tool and check-patch? For instance, clang-format has a feature to ignore broken lines. This could then be handled by check-patch, to allow more complex rules.
Do we just want to keep check-patch and let authors / their editors format the code?
Isn't this what we have now? IMHO I find checkpatch warnings useful, though some code style things such as CamelCase don't get caught at all by it, and are sometimes merged in.
Do we want to rely (solely) on reviewers for format checking?
I believe some of the current checks are rather useful, so I would leave them in place. Some other style nits are better judged by humans, though.
Do we want to encourage reviewers to educate their fellows on code style (for instance, wrt. line length, less indentation levels, shorter, more meaningful identifiers, etc.)?
If we get to agree on one code style, yes.
Nico
Do note this is my personal, uneducated opinion on the matter.
Best regards,
Angel Pons
Hi Nico,
(I Cc'd a few folks to make sure this bubbles up in their mailboxes because they engaged in that type of discussion which made this current round necessary)
Thanks for starting this thread. Let's make sure that it's the last one on this topic.
Nico Huber nico.h@gmx.de schrieb am Sa., 16. März 2019, 16:32:
Do we want to enforce a single editor / IDE + configuration for coreboot contributions? For instance, Vim is quite configurable and helpful when writing code. This could make all tools for later processing unneces- sary.
Practically speaking: that's not going to happen. There are attempts to define cross editor configs though: https://editorconfig.org, maybe that could help.
Do we want to enforce a single tool, e.g. clang-format, that does the
job for us after editing a source file?
If we go for strict coding style adherence requirements that would avoid having to manually review for coding style, which means less talking about it, which in my book is a plus.
The above, even if that implies a new coding style that we might not
be used to?
Whatever we have is at most a recommendation for now. Unless we keep coding style enforcement at the advisory level, things will have to change.
Do we want a combination of such a tool and check-patch?
Checkpatch is a set of regular expressions and as such we should rather try to get away from it as that is rather brittle by design. Since that will spark discussions about this every time the tool breaks, is rather get rid of it to the extent possible.
Combining multiple tools that weren't build for working together will only show the differences in their interpretation of the rules. We saw that already.
More generally speaking, I don't care on what we agree here, I just don't want to hear about that crap anymore, so please let's agree on something.
Having tools normalize the tree looks like an opportunity to reduce the amount of talk about coding style even more, so there's a weak preference to that on my part solely because of that. And yes, that means that I also don't care what "compliant" source files look like in the end: if there's agreement to standardize on GNU style C with a strict line length of 40 characters, I'll be fine with that just the same.
Patrick
On Sat, Mar 16, 2019 at 9:41 AM Patrick Georgi pgeorgi@google.com wrote: o Huber nico.h@gmx.de schrieb am Sa., 16. März 2019, 16:32:
Do we want to enforce a single editor / IDE + configuration for coreboot contributions?
we don't want to lock out, e.g., sublime, emacs, and vscode users, so no.
Do we want to enforce a single tool, e.g. clang-format, that does the job for us after editing a source file?
If we go for strict coding style adherence requirements that would avoid having to manually review for coding style, which means less talking about it, which in my book is a plus.
This is the direction most new projects in modern languages are taking. clang-fmt can do this for older languages like C and I think it makes the most sense.
Do we want a combination of such a tool and check-patch?
checkpatch can't do what it wants to do, i.e. parse C with REs, and so we get 5000 lines of Perl RE code chock full of false positives. So I'd prefer not.
More generally speaking, I don't care on what we agree here, I just don't want to hear about that crap anymore, so please let's agree on something.
yeah.
It's easy. Hand it all off to an automated formatter, don't insist on manual overrides for individual personal preferences, and, above all:
--> accept that the code will not always look the way you think it should look.
This is the part people get stuck on.
ron
Am Sa., 16. März 2019 um 18:16 Uhr schrieb Ron Minnich rminnich@google.com:
More generally speaking, I don't care on what we agree here, I just don't want to hear about that crap anymore, so please let's agree on something.
yeah.
It's easy. Hand it all off to an automated formatter, don't insist on manual overrides for individual personal preferences, and, above all:
--> accept that the code will not always look the way you think it should look.
That's one solution.
I'd also be fine with any other set of rules, incl. fully manual handling, or no coding style at all. I just want to not have to discuss it anymore :-)
Patrick
Hello,
After chatting a bit with Patrick on IRC I see why automated formatting would be much better than any manual formatting. If modern languages such as Go and Rust do it, it must be for some reason :-)
Am Sa., 16. März 2019 um 18:16 Uhr schrieb Ron Minnich rminnich@google.com:
--> accept that the code will not always look the way you think it should look.
Now that I see this, I fully agree. I'd rather see code that does things properly instead. I can't see how bikeshedding about code style is ever going to be productive.
Best regards,
Angel Pons
Do we want to enforce a single editor / IDE + configuration for coreboot contributions? For instance, Vim is quite configurable and helpful when writing code. This could make all tools for later processing unneces- sary.
I don't think this is something we'd ever want. It would be an enormous barrier to entry for new developers.
Do we want to enforce a single tool, e.g. clang-format, that does the job for us after editing a source file?
I'm not really a fan of auto-formatters because they can just never be as good as a human in all cases. The line length issue already shows that -- you're supposed to limit to 80 characters *unless* exceeding that significantly increases readability. There are certainly exceptions where that makes the code better to read, like when you have a giant table in a header where aligning equal elements in the same column really helps to see what is what (e.g. something like this: https://review.coreboot.org/cgit/coreboot.git/tree/src/mainboard/getac/p470/... ). There are also other alignment issues where tool output often doesn't look as good as handcrafted code, e.g. when you're aligning enum assignments at the equals sign or align the values in a long list of #defines to start on the same column. I understand that not everyone always wants to hand-align everything and it's not a submission requirement, but forcing a formatter means those who want to do that can't anymore, and the code looks worse for it. It also means we could never make a judgement call on a specific situation anymore if the formatter disagrees, no matter how much sense it would make or how unreadable the auto-formatted version is.
The main impetus for this is to allow people who don't want to worry about formatting to automate it away, right? Then why not make it optional? As I understand it, clang-format has options to analyze a patch and only reformat the lines that the patch touches. Why don't we just create a pre-commit hook with that which people can optionally install? Then you can run clang-format over your patches if you want to, and people who want to hand-format code can continue to do that. Nobody is going to complain about unrelated whitespace changes in patches if it only reformats the lines that the patch already touches. And in cases where we really want to format something different than the formatter wants to, it's easy to skip the hook.
Do we just want to keep check-patch and let authors / their editors format the code?
I think the current checkpatch system works pretty well, honestly. It catches most common mistakes and the false-positive rate is low and easy to ignore. I don't think we really have a problem that needs fixing in terms of code style enforcement -- in my reviewing experience, I rarely find myself having to point out style issues manually. At most I may be asking people to please pay attention to the checkpatch warnings, but usually they figure that out pretty quickly anyway. It's true that checkpatch is no full C parser, but it seems to be doing its job well enough and I don't see any drawbacks that are actively forcing us to move away from it. (If you're worried about excessive spam making reviews hard, maybe we can just add some sort of rate limit to the code that posts checkpatch results as comments? For example if there are more than 10 warnings of the same type, just post the raw checkpatch output for all of those in a single comment at the top of the file rather than each on the affected line. In my experience this is only a problem when patches are superbly borked anyway, such as when a file is full of DOS line endings.)
Do we want to encourage reviewers to educate their fellows on code style (for instance, wrt. line length, less indentation levels, shorter, more meaningful identifiers, etc.)?
Most of this is a normal part reviewing and will never be automated away anyway, right? E.g. I don't think we'll have a tool making good judgement calls on when indentation depth has become so deep that you should factor out another function or how to best name a variable anytime soon. I've always been looking at these things in my reviews and plan on continuing to do so.
Line length is the one thing that's easy to automate (at least as a hard limit), and we've already done that with checkpatch. So I guess it's checkpatch doing the educating, not the reviewer, but I think that works just as well. All the reviewer has to do is to make sure the checkpatch warnings have been addressed before submission (either by fixing them or by agreeing that ignoring the warning is fine in that case).
Am Di., 19. März 2019 um 21:53 Uhr schrieb Julius Werner jwerner@chromium.org:
I'm not really a fan of auto-formatters because they can just never be as good as a human in all cases.
As I understand Ron's argument, the idea is to accept being "less good" than any single expert person, because it's traded in for more consistency and less friction over everything code formatting since the Machine Is Right[tm].
Patrick
traded in for more consistency and less friction over everything code formatting
But this sounds like a pretty theoretical problem to me. Do we regularly have review friction over the kinds of issues that humans and formatter might disagree about (e.g. how to exactly align a continuation line)? In my experience those are just left to the author as long as it looks somewhat sane. The only style comments I usually see on reviews are unambiguous things like line length or brace placement, where the formatter will do the right thing when it's configured right.
Why not try making it optional first and see what friction remains in practice?
On Wed, Mar 27, 2019 at 6:13 AM Patrick Georgi pgeorgi@google.com wrote:
Am Di., 19. März 2019 um 21:53 Uhr schrieb Julius Werner jwerner@chromium.org:
I'm not really a fan of auto-formatters because they can just never be as good as a human in all cases.
As I understand Ron's argument, the idea is to accept being "less good" than any single expert person, because it's traded in for more consistency and less friction over everything code formatting since the Machine Is Right[tm].
I repeat Rob Pike's comment: "nobody likes the output of the Go formatter, everyone likes the Go formatter"
meaning that while the output doesn't always meet everyone's standard of perfection, it removes the arguments based on people's different ideas of what is good. And, plus, none of you agree with me or even each other in ALL cases on what "looks good", so at some point these arguments boil down to "because I like it."
I watched this roll out in the Go community in 2011 or so. gofmt was required. There were lots of arguments. In the end, people realized that it was nice to delegate formatting to a robot, because these arguments get exhausting.
Nobody now remembers a time when robots did not format Go code. Nobody wants to go back.
This is true of many projects, in particular those using Rust or Go.
ron
I have make quiet some mistake on coding style before and had been point out several times on review :-). I still fell like an automatic formatting can help myself and new comers.
Ron Minnich via coreboot coreboot@coreboot.org 于2019年3月27日周三 上午10:03写道：
On Wed, Mar 27, 2019 at 6:13 AM Patrick Georgi pgeorgi@google.com wrote:
Am Di., 19. März 2019 um 21:53 Uhr schrieb Julius Werner <
jwerner@chromium.org>:
I'm not really a fan of auto-formatters because they can just never be as good as a human in all cases.
As I understand Ron's argument, the idea is to accept being "less good" than any single expert person, because it's traded in for more consistency and less friction over everything code formatting since the Machine Is Right[tm].
I repeat Rob Pike's comment: "nobody likes the output of the Go formatter, everyone likes the Go formatter"
meaning that while the output doesn't always meet everyone's standard of perfection, it removes the arguments based on people's different ideas of what is good. And, plus, none of you agree with me or even each other in ALL cases on what "looks good", so at some point these arguments boil down to "because I like it."
I watched this roll out in the Go community in 2011 or so. gofmt was required. There were lots of arguments. In the end, people realized that it was nice to delegate formatting to a robot, because these arguments get exhausting.
Nobody now remembers a time when robots did not format Go code. Nobody wants to go back.
This is true of many projects, in particular those using Rust or Go.
ron _______________________________________________ coreboot mailing list -- coreboot@coreboot.org To unsubscribe send an email to coreboot-leave@coreboot.org
Ron Minnich via coreboot wrote:
Nobody now remembers a time when robots did not <insert task>. Nobody wants to go back.
I'll make you a T-shirt with this quote, Ron.
//Peter
On 16.03.19 18:15, Ron Minnich wrote:
On Sat, Mar 16, 2019 at 9:41 AM Patrick Georgi pgeorgi@google.com wrote: o Huber nico.h@gmx.de schrieb am Sa., 16. März 2019, 16:32:
Do we want to enforce a single editor / IDE + configuration for coreboot contributions?
we don't want to lock out, e.g., sublime, emacs, and vscode users, so no.
yeah, that was just an example, because Vim works for me. And usually the argument for code formatters seems to be that they work for some- body.
Why I brought this IDE point up at all: I played with the idea of a separate tool for formatting in my head and I couldn't come up with anything that would fit into my usual code+commit+push workflow. Maybe that's just because I'm not experienced with such tools. I wonder, at what point it would fit in:
o Between editor and `git add`? o As a pre-commit hook? o As a pre-push hook? o As a hook on Gerrit's side?
I couldn't come up with anything that wouldn't result in more manual work, e.g. to synchronize my local tree with the resulting commits. And well, I guess the point is to save us some work (I don't see where but probably somebody does).
So before we make any final call for a formatting tool, I guess we need a volunteer who says they're going to take the week (or month?) off to integrate it flawlessly (unless I miss here how easy it really is).
Do we want to enforce a single tool, e.g. clang-format, that does the job for us after editing a source file?
If we go for strict coding style adherence requirements that would avoid having to manually review for coding style, which means less talking about it, which in my book is a plus.
Well, I hope it will be a very huge plus. In my book it's at an incredible minus atm: Over the past 7 years of coreboot development, I've spent a lot more time to analyze or work around and discuss our check-patch/check-style hooks than I spent on bikeshedding code format or debating about line lengths.
This is the direction most new projects in modern languages are taking. clang-fmt can do this for older languages like C and I think it makes the most sense.
Most modern languages are parsable, I guess? I haven't tried it yet, but I guess modern languages are much easier to format automatically.
Nico
The general usage pattern in Go is format-on-save-from-editor, unless it's something like vscode where magic happens.
On Thu, Mar 28, 2019 at 2:03 PM Nico Huber nico.h@gmx.de wrote:
On 16.03.19 18:15, Ron Minnich wrote:
On Sat, Mar 16, 2019 at 9:41 AM Patrick Georgi pgeorgi@google.com wrote: o Huber nico.h@gmx.de schrieb am Sa., 16. März 2019, 16:32:
Do we want to enforce a single editor / IDE + configuration for coreboot contributions?
we don't want to lock out, e.g., sublime, emacs, and vscode users, so no.
yeah, that was just an example, because Vim works for me. And usually the argument for code formatters seems to be that they work for some- body.
Why I brought this IDE point up at all: I played with the idea of a separate tool for formatting in my head and I couldn't come up with anything that would fit into my usual code+commit+push workflow. Maybe that's just because I'm not experienced with such tools. I wonder, at what point it would fit in:
o Between editor and `git add`? o As a pre-commit hook? o As a pre-push hook? o As a hook on Gerrit's side?
I couldn't come up with anything that wouldn't result in more manual work, e.g. to synchronize my local tree with the resulting commits. And well, I guess the point is to save us some work (I don't see where but probably somebody does).
So before we make any final call for a formatting tool, I guess we need a volunteer who says they're going to take the week (or month?) off to integrate it flawlessly (unless I miss here how easy it really is).
Do we want to enforce a single tool, e.g. clang-format, that does the job for us after editing a source file?
If we go for strict coding style adherence requirements that would avoid having to manually review for coding style, which means less talking about it, which in my book is a plus.
Well, I hope it will be a very huge plus. In my book it's at an incredible minus atm: Over the past 7 years of coreboot development, I've spent a lot more time to analyze or work around and discuss our check-patch/check-style hooks than I spent on bikeshedding code format or debating about line lengths.
This is the direction most new projects in modern languages are taking. clang-fmt can do this for older languages like C and I think it makes the most sense.
Most modern languages are parsable, I guess? I haven't tried it yet, but I guess modern languages are much easier to format automatically.
Nico
-
Angel Pons -
Julius Werner -
Lance Zhao -
Nico Huber -
Patrick Georgi -
Peter Stuge -
Ron Minnich