Change in coreboot[master]: Documentation/coding_style: Issues not mentioned and cleanup patches
Attention is currently required from: Nico Huber, Martin Roth, Paul Menzel, Angel Pons, Ron Minnich, HAOUAS Elyes. Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/50966 )
Change subject: Documentation/coding_style: Issues not mentioned and cleanup patches ......................................................................
Patch Set 1:
(5 comments)
Commit Message:
https://review.coreboot.org/c/coreboot/+/50966/comment/3f9511f3_d5c748b3 PS1, Line 7: Issues not mentioned and cleanup patches
Nit: It’d be great if you made this a statement.
I already had trouble coming up with a good subject that fits 72 characters as it is. Feel free to make a concrete suggestion but this was the most expressive I could find.
File Documentation/coding_style.md:
https://review.coreboot.org/c/coreboot/+/50966/comment/47f4468d_e983fed4 PS1, Line 22: improvement. By default the style choices of the original author should
IMHO, another valid reason to clean up code style is when deduplicating stuff. […]
Sure, I would not consider that a "cleanup patch" in this sense because deduplicating is an actual code change, not just a style change. Of course when you're deduplicating multiple different versions into one, you get to make the style choices for the new combined version.
Do you think I need to add any specific text here to make that clearer? I would consider that implied already but happy to take suggestions.
https://review.coreboot.org/c/coreboot/+/50966/comment/7a67299d_53ab472f PS1, Line 27: bulk-applied to change existing code.)
New contributors should read this, though.
Yeah... I mean, I agree that there's plenty room for improvement in this document (the text is ancient, copied from a different project and pretty rambling in many places, after all... I think a simple, to the point "this is how you're supposed to do things" style with the explanations hidden in collapsible sections might be more readable). But I don't think "abandon this document and write a new one" can be a solution. We need to have a single coding style doc that reviewers can point to. We should fix the issues in this one rather than having a confusing mess of different documents side by side.
https://review.coreboot.org/c/coreboot/+/50966/comment/14db7857_7bf17d92 PS1, Line 24: `clang-format`. These tools can be useful to find : potential issues or simplify formatting in new submissions, but they : were not designed to directly match this guide and they should not be : bulk-applied to change existing code.
There are ideas to enforce clang-format across the tree (the crossgcc version to keep everybody on t […]
Well, I think you know how I feel about that, and as far as I'm aware there has been no agreement to make clang-format mandatory yet. I don't think anyone can seriously say that changes like https://review.coreboot.org/c/coreboot/+/50853/1/src/soc/rockchip/rk3399/sdr... actually make the code better, and I have personally not seen this particular style issue (continuation line indentation) being much quibbled about either. It's just something that many authors like to do on their own to make their code easier to read for themselves and others, and I don't see why we should force them not to.
Anyway, I'm just trying to explain the current situation here right now. This file can always be updated again when and if there's a new consensus on clang-format.
https://review.coreboot.org/c/coreboot/+/50966/comment/62d8aadd_9adc7379 PS1, Line 27: bulk-applied to change existing code.)
Please mention that checkpath.pl and clang-format can report false-positives.
Yes, that was basically what I'm trying to say here. Clarified a bit in new version.
-
Julius Werner (Code Review)