Attention is currently required from: Angel Pons, Nikolai Artemiev, Sergii Dmytruk.

Patch set 53:Code-Review +1

View Change

7 comments:

• Patchset:

  • Patch Set #53:

    Ah, sorry, apparently I forgot to publish comments the last
    time I looked ._.

• File libflashrom.h:

  • Patch Set #49, Line 153: range_list

    Maybe abbreviate here and in the two functions below like above, i.e.
    `ranges`?

  • Patch Set #49, Line 154: size_t

    I would prefer `unsigned int` as `size_t` is officially for byte counts.
    We use it internally sometimes for indices in general, but it seems
    better to not have such local style in the API.

• File libflashrom.c:

  • Patch Set #49, Line 800: }

    No need to change this if you don't want to. The pattern that seems
    to prevail nowadays is to check for errors first and bail out, so
    the actual code is at the end and doesn't need indentation, i.e.

      if (index >= list->count)
              return FLASHROM_WP_ERR_OTHER;

      *start = ...

  • Patch Set #49, Line 811: free(list->ranges);

    Better to check for NULL first, e.g.

      if (!list)
              return;

    Then the caller doesn't need to ensure it's set. free() allows NULL and
    it makes error paths easier to handle when one just needs to write
    `free(x);` without needing to care if `x` was allocated already.

    (flashrom_flash_release() doesn't follow that rule either, I'll push a patch CB:62340)

• File writeprotect.c:

  • Patch Set #49, Line 171: Comparitor

    Compar*a*tor

  • Patch Set #49, Line 200: size_t j = a->bits.bp_bit_count - i - 1;

    Reversing the loop would be more readable, I guess:

      for (int i = a->bits.bp_bit_count - 1; i >= 0; --i)

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