Attention is currently required from: Nico Huber, Edward O'Callaghan, Light, Anastasia Klimchuk.
7 comments:
File pony_spi.c:
nit: add space before opening brace
Patch Set #3, Line 167: bool have_device;
From my point of view: Review doesn't scale well with the size of
a patch. Even if it's a very simple patch hunk, every additional
hunk can distract from the error-prone part of a patch. Doing fixes
and simple changes like this in advance works well, FWIW.
+1
For me, it's very easy to miss something when reviewing commits that do multiple things at once. I'd rather review multiple commits where each does one and only one thing.
Whenever I want/need to make significant code changes, I draft in advance a "plan of action". I use several criteria to segment the changes into commits, one of them is "at most one thing/reason/idea per commit".
In this case, "simplify `pony_spi` init logic" and "use `bool` type for boolean variables" are two different things, and they are conceptually independent: disregarding merge conflicts, it's possible to do them in either order, or only do one of them. When accounting for merge conflicts, I start with the easiest (most likely to be approved) commits, so that I don't have to manually rebase them if another commit needs to be fixed.
That being said, there's no need to over-split things. For example, I'd retype both `have_device` and `have_prog` in the same commit, because the rationale is the same.
TL;DR: Please retype the variables in a separate commit (preferably before the "extract `get_params`" commit to streamline the submission process).
Patch Set #3, Line 171: return 1;
If the `dev` parameter is correct (i.e. `have_device` is true) but `get_params()` returns -1 because of an error with the `type` parameter, the serial port (what `serialport_shutdown()` releases) will leak.
Patch Set #3, Line 181: return 1;
Serial port can also leak here.
Patch Set #3, Line 190: serialport_shutdown
I think you are mis-interpreting the diff presented by gerrit Anastasia.
This line is OK here, but it's missing in some other return paths.
This is unrelated. Please do it in a separate commit.
Ditto
To view, visit change 63130. To unsubscribe, or for help writing mail filters, visit settings.