-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
On 12/20/2013 02:31 PM, Stephen Gallagher wrote:
On 12/20/2013 11:15 AM, Nikolai Kondrashov wrote:
> Hi everyone,
> Please find attached the third version of the DEBUG macro
> refactoring patchset. The second version was lost to the
> maillist message size limit, so I'll list the changes from the
> first one:
> * level limiting condition moved to the macro as requested by
> Simo, * macro definition update is separated from the invocation
> update, * automatic macro invocation update is separated from
> the following manual fixups, * invocations using the old debug
> level (number literals) are updated to use the new bitmask
> macros, * the level conversion function "debug_get_level" is
> removed.
> Please feel free to pick and squash commits as necessary.
> Sincerely, Nick
Patch 0001: Ack Patch 0002: Ack Patch 0003: Ack Patch 0004: Ack
Patch 0005: Nack Patch 0006: Nack
I'd rather that 0005 and 0006 be squashed together and that the
comment be written as follows:
{{{ Make DEBUG macro invocations variadic
Use a script to update DEBUG macro invocations to use it as a
variadic macro, supplying format string and its arguments directly,
instead of wrapping them in parens.
This script was used to update the code:
grep -rwl --include '*.[hc]' DEBUG . | while read f; do mv
"$f"{,.orig} perl -e \ 'use strict; use File::Slurp; my
$text=read_file(\*STDIN);
$text=~s#(\bDEBUG\s*\([^(]+)\((.*?)\)\s*\)\s*;#$1$2);#gs; print
$text;' < "$f.orig" > "$f" rm "$f.orig" done
There exists a single non-automated change in this patch, due to
the script being slightly too greedy in two places.
1. The definition of the DEBUG() macro matched incorrectly, causing
the parser to greedily continue advancing until it eventually found
a double-closing-parenthesis in the talloc_zfree() macro
definition.
2. The presence of a comment containing the literal string
"DEBUG()" caused the parser to incrorectly match and proceed to the
next valid DEBUG() macro usage, removing the trailing parenthesis
but missing the leading one. }}}
That being said, the logic seems correct everywhere else. Nikolai,
if you agree with this, no need to resend the patches. This is
easily adjusted before we push, so you can consider it an Ack.
Patch 0007: The conversion seems *correct*, if not ideal. I don't
love that it effectively makes all of our DEBUG lines exceed our
79-character limit. I don't suppose you could amend your patch to
reflow things using our current convention:
DEBUG(SSSDBG_TRACE_ALL, "Some words go here\n", arg1, arg2);
I'm not willing to block the patch on this, however. Particularly
since a non-trivial number of existing DEBUG messages were already
over the limit.
One other comment I seem to recall from the earlier (long) thread
that I'd like to address. It was brought up that the reason we were
leaving these alone was to deal with them only when we touched the
code they to which they were related. I think by doing this
mass-conversion, it becomes much easier to spot at a glance when a
message is poorly categorized. It makes it possible for a new
contributor to come in and fix up these levels, even when not
working on other code in the area.
So I'm in favor of making this change, and best to do it all at
once and rip off the band-aid.
Consider 0007 acked, unless Nikolai wants to try his hand at
reflowing it.
Patch 0008: I'm not sure we want to drop this compatibility
without checking with all of our downstream packagers (and anyone
who is building with custom patches). It's very possible that they
may be carrying forward old patches that still retain the 0-9 debug
level values and breaking them might be unwelcome.
At the very least, this would need to be documented loudly in the
release notes for the version in which this change occurs.
Actually, minor Nack on 0008:
#define DEBUG(level, format, ...) do { \
- - int __debug_macro_newlevel = debug_get_level(level); \
- - if (DEBUG_IS_SET(__debug_macro_newlevel)) \
- - debug_fn(__FUNCTION__, __debug_macro_newlevel, format,
##__VA_ARGS__); \
+ int __debug_macro_level = level; \
+ if (DEBUG_IS_SET(__debug_macro_level)) \
+ debug_fn(__FUNCTION__, __debug_macro_level, format,
##__VA_ARGS__); \
} while (0)
If you're going to drop debug_get_level(), there's no reason to use
_debug_macro_level at all here. Drop that too.
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.15 (GNU/Linux)
Comment: Using GnuPG with Thunderbird -
http://www.enigmail.net/
iEYEARECAAYFAlK0ndQACgkQeiVVYja6o6MaaQCeNCbb6fJSgRI5rkSljJj8YrKv
AAIAnjmU04UxvgDjMpH4RKNJk/vqYlhJ
=xlck
-----END PGP SIGNATURE-----