David Hendricks has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/34363 )
Change subject: libflashrom: add querying functions with meson integration ......................................................................
Patch Set 3:
(6 comments)
In the interest of time, I went ahead and made some minor changes based on Angel's comments. I'm not sure what is the right thing to do with flashrom_data_free(), though.
https://review.coreboot.org/c/flashrom/+/34363/2/libflashrom.h File libflashrom.h:
https://review.coreboot.org/c/flashrom/+/34363/2/libflashrom.h@41 PS2, Line 41: typedef
Oh no, typedefs
Agreed, I prefer not to use typedefs unless they're really needed. So I got rid of 'em.
https://review.coreboot.org/c/flashrom/+/34363/2/libflashrom.h@49 PS2, Line 49: typedef
There's more! O_o
Done
https://review.coreboot.org/c/flashrom/+/34363/2/libflashrom.h@53 PS2, Line 53: struct flashrom_tested { : flashrom_test_state probe; : flashrom_test_state read; : flashrom_test_state erase; : flashrom_test_state write; : } tested;
enums on structs?
This is the same as in flash.h...
https://review.coreboot.org/c/flashrom/+/34363/2/libflashrom.c File libflashrom.c:
https://review.coreboot.org/c/flashrom/+/34363/2/libflashrom.c@109 PS2, Line 109: Flashrom
flashrom in lowercase
Done
https://review.coreboot.org/c/flashrom/+/34363/2/libflashrom.c@116 PS2, Line 116: /** : * @brief Returns list of supported programmers : * @return List of supported programmers : */
I don't think the two lines in this comment should say the same. […]
Fixed here, and other instances.
https://review.coreboot.org/c/flashrom/+/34363/2/libflashrom.c@233 PS2, Line 233: int flashrom_data_free(void *const p)
Do you really need this function? free()'ing NULL is a valid noop (https://pubs.opengroup. […]
On second thought, it's probably good to have this function in the API. However I wonder if p == NULL should really be considered an error. Thoughts?