Re: [PATCH] debug prints

On 02/18/2013 04:39 AM, Amos Jeffries wrote:
> On 10/08/2012 1:57 p.m., Amos Jeffries wrote:
>> On 9/08/2012 12:12 a.m., Alexander Komyagin wrote:
>>> Following patch fixes nested debug() calls problem.
>>> Since debugs() is a macros, it should not change static Debugs::level
>>> before putting debug message to the internal stream. Otherwise we
>>> encounter problems when debug message itself contains calls to debugs().
>>>
>>> --- src/Debug.h 2012-03-07 05:42:55.000000000 +0300
>>> +++ src/Debug.h 2012-08-08 14:49:20.000000000 +0400
>>> @@ -106,8 +106,9 @@
>>> /* Debug stream */
>>> #define debugs(SECTION, LEVEL, CONTENT) \
>>> do { \
>>> - if ((Debug::level = (LEVEL)) <= Debug::Levels[SECTION]) { \
>>> + if ((LEVEL) <= Debug::Levels[SECTION]) { \
>>> Debug::getDebugOut() << CONTENT; \
>>> + Debug::level = (LEVEL); \
>>> Debug::finishDebug(); \
>>> } \
>>> } while (/*CONSTCOND*/ 0)
>>>
>>
>> Looks okay. How much testing has this had?
>>
>> Amos
>
> Applied as trunk rev.12698. Sorry this took so long to go in.

Please note that debugs() is not the only macro that changes
Debug::level before doing anything else. The old do_debug() does that as
well.

debugs() still changes Debug::sectionLevel before we do anything else so
there is inconsistency now with regard to level and sectionLevel.

Finally, this change makes current debugging level unavailable to
CONTENT. I know some CONTENT uses Debug::sectionLevel. I do not see any
CONTENT using Debug::level, so this may not be a problem today.

I cannot tell exactly what problems this change is fixing (the commit
message does not explain), so the fix may very well be worth the
side-effects, but it looks like the debugging code became messier after
this change. I wonder if the fix is worth it and whether there is a
better way to fix that problem?

Thank you,

Alex.
Received on Mon Feb 18 2013 - 20:22:24 MST