View Change

2 comments:

• File flashrom.c:

  • Patch Set #1, Line 811:

    	if (c == '1') {
    		*value = 1;
    		return 0;
    	}
    
    	if (c == '0') {
    		*value = 0;
    		return 0;
    	}
    

    I don't think a switch statement would help. […]

    np. My mind was leaning towards 'true' and 'false' maybe need to be handled as well for other spi masters to make full use of this across the tree. We also wind up with a much more consistent param format for each using your solution. Feel free to just ACK of course, the common theme here is to realise the full benefits of this work.

• File programmer.h:

  • Patch Set #1, Line 615: int extract_programmer_param_toggle(const char *param_name, int *value);

    I can convert other files in follow-ups (I don't want this change to grow a lot). […]

    In regards to the former point, I would argue to split the change into two parts the first that introduces the helper (ideally with a basic test) and the second that switches all the relevant spi masters to the new helper framework atomically. It is much better than these ad hoc framework changes and lines up with the advise you gave me about getting the write-protect framework right step by step before merge.

    In regards to unit-tests, the latter point about preventing software rot is precisely the point of the unit-test. Unit-tests are typically a poor choice of tool for theorem proving an initial implementation as you correctly eluded to. The unit-test could just be a simple few cases, calling the fn iterated over an array of param combos and checking the return value:

    ```
     i)   (NULL, NULL) => -1,
     ii)  ("str", NULL) => -1,
     iii) (NULL, &[int x]) => -1,
     iv)  ("p=", &[int x]) => 0?,
     v)   ("p=1", &[int x]) => {0 && x=1},
     vi)  ("p=0", &[int x]) => {0 && x=0},
     vii) ("p=o", &[int x]) => -1,
    ```

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