Change in flashrom[master]: [WIP] replace programmer enum with a pointer to programmer_entry

[Nico Huber (Code Review)]

Attention is currently required from: Thomas Heijligen.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/51925 )

Change subject: [WIP] replace programmer enum with a pointer to programmer_entry
......................................................................


Patch Set 4:

(5 comments)

This change is ready for review.

Patchset:

PS4: 
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:

https://review.coreboot.org/c/flashrom/+/51925/comment/de8b970a_e4e10051 
PS4, 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.


https://review.coreboot.org/c/flashrom/+/51925/comment/5e7672d3_809b9a64 
PS4, 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:

https://review.coreboot.org/c/flashrom/+/51925/comment/f6a76ee5_276acf11 
PS4, 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:

https://review.coreboot.org/c/flashrom/+/51925/comment/5f85e505_4c9930cc 
PS4, 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 https://review.coreboot.org/c/flashrom/+/51925
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings

Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I7435ee2bb661e4f139034bb289b84ebd41e1e9e9
Gerrit-Change-Number: 51925
Gerrit-PatchSet: 4
Gerrit-Owner: Thomas Heijligen <[e-mail]>
Gerrit-Reviewer: Angel Pons <[e-mail]>
Gerrit-Reviewer: Nico Huber <[e-mail]>
Gerrit-CC: Xiang Wang <[e-mail]>
Gerrit-Attention: Thomas Heijligen <[e-mail]>
Gerrit-Comment-Date: Wed, 31 Mar 2021 14:33:30 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment