Attention is currently required from: Felix Singer, Nico Huber, Angel Pons.
Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/66892 )
Change subject: ichspi.c: Retype appropriate variables with bool ......................................................................
Patch Set 13: Code-Review+2
(1 comment)
File ichspi.c:
https://review.coreboot.org/c/flashrom/+/66892/comment/584edf2a_648f7087 PS7, Line 872: write_cmd = true;
Thanks Angel for your comment! You summarized the situation very good. […]
As a reviewer who went through the whole chain, I can say IMO:
1) It was easier to review as a chain of smaller patches (as it is now) VS one gigantic patch. I appreciate patch owner did that work. Thank you!
2) My understanding that changing the type int -> bool changes the binary, so commits are not reproducible, doesn't matter whether changes are split or combined.
3) Renaming only comes up for this one file (two variables in here), not for every patch in the chain. I read this comment on my first round of review and payed attention to variables names. I didn't notice anything obvious to rename. Which means, in this specific situation, renaming in a separate patch does not mean twice more patches, it means one more patch.
4) In addition to 1), focusing the chain on one thing, "retyping to bool" makes it easier to review.
5) Changing this one patch now to do two things (retype and rename) unfortunately will create more work for patch owner and reviewers, because this version is already rebased, reviewed and tested.
The best thing: everyone has the same end goal in mind: "bool variables with meaningful names" ! :) As I said earlier, I agree with renaming `is_write_command`, it's a good idea.
-
Anastasia Klimchuk (Code Review)