Hi Jakub,
Thanks a lot for reviewing!
On 01/07/2014 02:33 PM, Jakub Hrozek wrote:
I spoke about the same problem with Simo (or Stephen? Sorry, I
forgot
over holidays) and it seems my opinion is in minority so I won't block
the patchset over it.
I have some very minor comments about the patches, see below:
[PATCH 2/8] Move DEBUG macro body to debug_fn
Mostly ACK, I just have a question about debug_fn() -- while I realize
the code was taken from the old DEBUG macro, now that it's a function,
maybe you could reflow it so it's more in line with our coding style and
better readable as a result. In particular, there could be a newline after
declaration of "va_list ap" and before call to "va_start".
Similarly, the block executed after testing debug_timestamps() could be its
own functions, as in general we don't do intermixed code and declarations.
But this is mostly my personal preference, again, not blocking the patches.
I think a clearer approach would be to reflow it in a separate patch. I'll see
if I have time for this. I wouldn't mind someone else fixing it, though.
[PATCH 3/8] Remove extra flushing from debug message output
The code is fine, but why is it a separate patch and not part of #2 ? It
seems the issue was introduced in patch 2/8, right?
No, it was there before my changes, in the original "debug_fn" function, which
body was moved to the new "debug_vprintf". That's why I kept it separate.
However, please squash it, if you think it's right.
So in conclusion - ACK, I'll leave up to you whether you want to
change
formatting of debug_fn and/or squash 3/8 into 2/8.
Great work!
Thanks Jakub. I'm still open to job offers ;)
Sincerely,
Nick