Attention is currently required from: Thomas Heijligen.

View Change

5 comments:

• Patchset:

  • Patch Set #4:

    Looks quite good already. I think the DEFAULT_PROGRAMMER issue
    (and also the INTERNAL_PROGRAMMER checks) would solve itself
    if you would move the programmer_entry structs to the individual
    source files, first. e.g.

    • first commit: move programmer_entry, turn programmer_table into an array of pointers (leaving the enum stuff as is for now), and s/programmer_table[.*]\./programmer_table[.*]->/ everywhere.

    * second commit: introduce a default_programmer pointer. (const, set at compile time,
      replacing CONFIG_DEFAULT_PROGRAMMER).

    • third commit: this change, using `default_programmer` and `&internal_programmer` where needed.

• File cli_classic.c:

  • Patch Set #4, Line 379: size_t _i

    Common flashrom style is to avoid such `for` local declarations. What I usually
    say here: the style should be discussed on the mailing list (and I really wouldn't
    mind).

    Anyway, you can just use `i` here.

  • Patch Set #4, Line 383: prog = &programmer_table[_i];

    This needs to be moved to the positive cases below (':' and '\0'). Otherwise we
    would turn this into a prefix match by accident. Or, when we hit the `continue;`
    below, it should be reset to `NULL`.

• File flashrom.c:

  • Patch Set #4, Line 2452: cb_check_image(newcontents, flash_size) < 0) {

    Should be indented with either 4 spaces or 2 tabs (shouldn't match with the
    body). Or don't break the line: our limit is 112 chars[1].

    [1] https://flashrom.org/Development_Guidelines#Coding_style

• File libflashrom.c:

  • Patch Set #4, Line 128: }

    This is quite weird. It leaves the last two pointers uninitialized.
    The API seems also meaningless, there is no way to know how many
    items are returned.

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