Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/46717 )
Change subject: Introduce `extract_programmer_param_toggle` helper ......................................................................
Patch Set 2:
(3 comments)
https://review.coreboot.org/c/flashrom/+/46717/1/flashrom.c File flashrom.c:
https://review.coreboot.org/c/flashrom/+/46717/1/flashrom.c@803 PS1, Line 803:
delete spurious \n
Strictly speaking, it's not spurious, since I decided to add it here. But I've dropped it.
Please don't get me wrong, though: I'm not angry about it at all, it's just that I couldn't care less about it. I recently decided that minor nits don't deserve anyone's time to be wasted on. Given that code style is something nearly everyone can comment on, and that making everyone happy about it is impossible, trying to argue about it only inflicts pain and misery upon the involved. Thus, I now address these situations and move on as quickly as possible.
And yes, writing this took me a while, but I'm Weird and I'd rather explain myself than risk being misunderstood. :)
Done
https://review.coreboot.org/c/flashrom/+/46717/1/flashrom.c@811 PS1, Line 811: if (c == '1') { : *value = 1; : return 0; : } : : if (c == '0') { : *value = 0; : return 0; : }
seems like a switch would make this tidy?
I don't think a switch statement would help. Currently, there's only two cases to handle, so using a switch statement doesn't make much of a difference.
If more possible values get added later on, then one would have to update the error message below accordingly. If using a switch statement, one would need to write down the possible cases twice: once for the switch statement, and once for the error message.
https://review.coreboot.org/c/flashrom/+/46717/1/programmer.h File programmer.h:
https://review.coreboot.org/c/flashrom/+/46717/1/programmer.h@615 PS1, Line 615: int extract_programmer_param_toggle(const char *param_name, int *value);
I like this but I feel flashrom has suffered from many half done solutions in the past. […]
I can convert other files in follow-ups (I don't want this change to grow a lot).
Regarding unit tests, I'm not sure how I would write tests for this function. I believe I've handled all the possible scenarios that this function could run into, so any test cases I'd come up with would be testing the same things I've accounted for. While that would help prevent software rot, it would not handle bugs in the original implementation.