> 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/irq_tables.c#n32 ). 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). _______________________________________________ coreboot mailing list -- coreboot@coreboot.org To unsubscribe send an email to coreboot-le...@coreboot.org
