On Wed, Dec 25, 2013 at 02:10:20AM +0200, Nikolai Kondrashov wrote:
On 12/23/2013 01:42 PM, Jakub Hrozek wrote:
>On Sun, Dec 22, 2013 at 11:23:58AM +0200, Nikolai Kondrashov wrote:
>>
>> 1. There are 2273 uses of the old levels, *half* of all DEBUG macro
>> invocations, so updating them on occasion will take a *long* time.
>> 2. Code is more often read than written (modified), and all this time
>> developers will be reading code using obscure numeric literals, which
>> take additional mental hop (effort) to comprehend.
>> 3. Spotting incorrect debug level will be easier with symbolic levels
>> instead of numeric ones and thus they're more likely to be
corrected.
>
>Sorry, I disagree about 2) and 3). Maybe it's just how I work, but I've
>trained myself to convert any numeric constants I see in the code and
>just skip the SSSDBG_ unless I'm also touching the DEBUG macro for other
>reasons.
The new debug levels were introduced at the beginning of August 2011
(a32ae272). That makes it about 2.5 years ago. Even assuming update progress
is linear (which it isn't), it will take 2.5 years more to update the other
half. That means 2.5 years of reading code with extra effort for developers.
Although, admittedly, the less old code there is, the less effort there will
be (but that is not linear as well).
Sincerely,
Nick
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 1/8] Add cscope inverted index files to .gitignore
ACK
[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.
[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?
[PATCH 4/8] Make DEBUG macro definition variadic
ACK
[PATCH 5/8] Make DEBUG macro invocations variadic
ACK, although I haven't reviewed the patch too closely, I simply checked
couple of random messages.
[PATCH 6/8] Fixup DEBUG macro invocations update
ACK
[PATCH 7/8] Update DEBUG* invocations to use new levels
ACK (I tested the new version that was sent separately)
[PATCH 8/8] Remove debug_get_level as unnecessary
ACK
I'm not too afraid about breaking 3rd party patches. We can announce
this change on the devel list separately (w/o the [PATCH] keyword) and
the patches would break at build-time, so the breakage wouldn't affect
users, only packagers. They should be able to come for help if they
can't fix the patch themselves.
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!