Patch set 6:Code-Review +2

View Change

2 comments:

• File payloads/libpayload/include/sysinfo.h:

  • Patch Set #5, Line 110: #define UNDEFINED_STRAPPING_ID_64 (~(uint64_t)0)

    I tried that, gcc wasn't happy: […]

    Stupid warnings, why don't we just disable them... okay fair enough. ^^

    In that case, maybe it's better to name it UNDEFINED_FW_CONFIG or something similar to what you had earlier? That might be easier to follow than having to keep track of which value is 32-bit and which is 64-bit.

• File src/lib/coreboot_table.c:

  • Patch Set #1, Line 20: #include <version.h>

    If I ever get too annoyed by headers being out-of-order, I sort them beforehand.

    I makes it easier to binary search with your eyes and see if a specific header is already in there? Idk, it's something that many people have been implicitly doing and asking for on patches, it's not in the official coding style so if you don't think we should do it we could have that discussion as well. (I agree that the specific order in this patch set was weird, and when we sort we should sort everything other than keeping <> and "" headers separate.)

    I wanted to say that in a more general sense though, not just in relation to header sorting. People often mix trivial code cleanups that otherwise nobody would bother to do at all into functional patches that touch the surrounding area, and I think to a certain extent we should avoid discouraging that.

To view, visit change 46605. To unsubscribe, or for help writing mail filters, visit settings.