David Hendricks has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/42556 )
Change subject: [WIP] Add uncrustify config ......................................................................
Patch Set 2: Code-Review-1
(8 comments)
Most of the issues are minor at this point. Overall I'm optimistic about this approach!
There is a problem with empty comments in flashchips.c (L1322, for example), however the new version of gerrit seems too slow to be able to comment on that (?!). I'll look into it a bit more so we don't create superfluous newlines.
https://review.coreboot.org/c/flashrom/+/42556/2/cbtable.c File cbtable.c:
https://review.coreboot.org/c/flashrom/+/42556/2/cbtable.c@206 PS2, Line 206: ; This semicolon is not needed and should be removed from the original file.
https://review.coreboot.org/c/flashrom/+/42556/2/hwaccess.h File hwaccess.h:
https://review.coreboot.org/c/flashrom/+/42556/2/hwaccess.h@42 PS2, Line 42: #define ___constant_swab8(x) ((uint8_t)( \ These are a bit funky since the leftmost cast is on the first line. A couple of ways to make them easier for uncrustify to handle: - Put the cast on an indented newline - Make this into an inline function. IMO this is better since then we can avoid all these casts and have the compiler spit out an error if somehow the wrong type is input.
https://review.coreboot.org/c/flashrom/+/42556/2/ich_descriptors.h File ich_descriptors.h:
https://review.coreboot.org/c/flashrom/+/42556/2/ich_descriptors.h@184 PS2, Line 184: : 3, uncrustify does this for unnamed fields in bitfields. The easy workaround is to give each a name, e.g. rsvd1, rsvd2, etc.
https://review.coreboot.org/c/flashrom/+/42556/2/ich_descriptors.c File ich_descriptors.c:
https://review.coreboot.org/c/flashrom/+/42556/2/ich_descriptors.c@217 PS2, Line 217: static const char *const freq_str[3][8] = { { The indent in this case is based on the rightmost brace (for the first array of strings). Moving it to a newline sets it to the left.
https://review.coreboot.org/c/flashrom/+/42556/2/jlink_spi.c File jlink_spi.c:
https://review.coreboot.org/c/flashrom/+/42556/2/jlink_spi.c@227 PS2, Line 227: if (ret != JAYLINK_OK) { : msg_perr("jaylink_parse_serial_number() failed: %s.\n", jaylink_strerror(ret)); : free(arg); : return 1; : } This seems redundant with the preceding if-statement, and should be fixed in another patch.
https://review.coreboot.org/c/flashrom/+/42556/2/serial.c File serial.c:
https://review.coreboot.org/c/flashrom/+/42556/2/serial.c@112 PS2, Line 112: { BAUDRATE(0) can be used to make this a single line, if desired.
https://review.coreboot.org/c/flashrom/+/42556/2/spi25_statusreg.c File spi25_statusreg.c:
https://review.coreboot.org/c/flashrom/+/42556/2/spi25_statusreg.c@454 PS2, Line 454: msg_cdbg("no sectors are protected\n"); This is fixed by putting the case labels in parens, i.e. `case (0x0 << 2):`
https://review.coreboot.org/c/flashrom/+/42556/2/uncrustify.cfg File uncrustify.cfg:
https://review.coreboot.org/c/flashrom/+/42556/2/uncrustify.cfg@11 PS2, Line 11: to of