Nico Huber has posted comments on this change. ( https://review.coreboot.org/28707 )
Change subject: Remove unneeded whitespace and fix char line limit ......................................................................
Patch Set 4:
(6 comments)
Sorry, I gave up reviewing... if you want to remove all these line breaks (i.e. enforce to use all available space in the line), we should have clearer rules about what is good style first (I'll comment one oc- currence in `cbtable.c` where I think it looked better before).
There are cases that are obviously wrong (e.g. lines starting with spaces where a tab would fit, or mixed tabs and spaces). And I wouldn't object fixing those (and removing line breaks around these cases). I'm trying to keep the noise in the git history produced by such patches as low as possible.
But having to bikeshed style wastes too much time that is better spent on actual software development.
https://review.coreboot.org/#/c/28707/1/board_enable.c File board_enable.c:
https://review.coreboot.org/#/c/28707/1/board_enable.c@159 PS1, Line 159: uint8_t base; /* base register in that LDN for the port */
There's actually only 4 spaces after a tab... […]
I don't understand. Do you mean not "enough spaces after the tab" that could be removed? In any case we are far below 112 chars here.
https://review.coreboot.org/#/c/28707/4/board_enable.c File board_enable.c:
https://review.coreboot.org/#/c/28707/4/board_enable.c@1122 PS4, Line 1122: * overridden by connecting the two solder points next to F2. This change seems too arbitrary. I don't think there is a rule for this (whitespace amidst a line and not from the start) and it's not obvious to me which style is better. So the next per- son who wants to clean things up could argue that it should be spaces.
Unless you have a strong argument, please leave such manual formatting as is.
https://review.coreboot.org/#/c/28707/4/board_enable.c@1347 PS4, Line 1347: {0x4E, 0x0100, 0x0000}, /* ...GPO29: XBCS bit 8 */ Same here (and I don't see why it should be two tabs).
https://review.coreboot.org/#/c/28707/4/board_enable.c@2024 PS4, Line 2024: val &= ~0x87; /* Output, non-inverted, GPIO, push/pull */ And here.
https://review.coreboot.org/#/c/28707/4/board_enable.c@2240 PS4, Line 2240: * to the respective tables in print.c. Thanks! and here
https://review.coreboot.org/#/c/28707/4/cbtable.c File cbtable.c:
https://review.coreboot.org/#/c/28707/4/cbtable.c@a294 PS4, Line 294: Here, arguments were grouped (header in one line, table in another) making it more readable, IMO. But that might just be a question of taste.
-
Nico Huber (Code Review)