[Bug 41199] [patch] Enhance logging system

[bugzilla-daemon at freedesktop.org]

https://bugs.freedesktop.org/show_bug.cgi?id=41199

--- Comment #10 from Olli Salli <ollisal at gmail.com> 2011-10-04 03:30:00 PDT ---
(In reply to comment #9)
> I had to fight for hours against a stupid linkage problem (for some reason the
> linker doesn't like the extern function pointer in the unnamed namespace), but
> here[1] is a new patch that solves the problems checking if the string is empty
> instead of using shared pointers...

Yes, the whole intent of the unnamed namespace concept in C++ is to mangle the
symbol name in a compile unit (a .cpp file + what it includes, pretty much)
specific fashion. Hence, extern linking from another compile unit will fail to
find the symbol. The symbol will be in the .so, but its name will have some
random crap added to it and hence it won't be found (but it won't conflict with
symbols for other variables and functions with the same name either).

Also, there are other problems with sharing global variables between shared
libraries, in initialization, in particular ordering. That's one reason why I
suggested making the dtor call some small exported utility function which does
the actual job of invoking the function pointer with the message. The other is:

debug-internal.h

25    25    
26    #include "config-version.h"
26    27    

This will cause the whole library to be recompiled when just the library
version has been changed, because mostly everything includes debug-internal.h,
and config-version.h changes when the version changes. I specifically split the
version out of config.h so that this wouldn't happen. Note that there are two
version number bumps in our standard review procedure and a full tp-qt4
recompile takes 5-10 minutes, so a full recompile on each version number bump
is a rather frustrating issue.

This can be avoided if the exported utility function for invoking the ctor just
takes the message type and debug message contents as a param. Being implemented
in debug.cpp (but NOT in the anonymous namespace), it can access
config-version.h and hence still pass the library name and version without
debug-internal.h including that header.

40        inline Debug(const Debug &a) : type(a.type), debug(new QDebug(&msg))

If a doesn't have a debug object (i.e. debug is disabled at runtime and
enabledDebug() has used the default constructor), we have to prevent
constructing a QDebug instance and streaming the message to it, like the old
copy ctor did. This to avoid the prohibitive QDebug creation and destruction
costs. Otherwise, I like your idea to use the streaming operator to shovel the
old instance's current contents to the new one, as that ensures our debug
instance writes to the string correctly etc. THOUGH: please verify it doesn't
put "" around the original string as I think it might do. That would be ugly.

The assignment operator OTOH already looks good in this respect.

Otherwise looks good now, except that I don't see any way to restore the
default debug behavior after e.g. setting a temporary callback for the duration
of one operation in an application (or when some log capture option has been
enabled). While exporting defaultDebugCallback in the public debug header (why
does it need to be exported in the .so if it's just used for initializing the
function pointer in the .cpp btw? or does it?) would be one solution, I think
cleaner would be to document passing 0 (NULL) to setDebugCallback as restoring
the default behavior, as it has no sensible meaning otherwise.

defaultDebugCallback being installed public API wouldn't make much sense to me;
it being just a print formatting function.

This would also simplify the internals: you could just zero-init the debug
callback function pointer. The magic new function which invokes the user-set
debug callback if there is one, filling in the version info etc, would then
just do what defaultDebugCallback being the callback currently does if the set
callback is 0.

That'd also be, again, faster, because it doesn't go through the relocation
table. Making defaultDebugCallback a non-exported inline function in the anon
ns in debug.cpp (only present when debugging is enabled at compile time) would
also achieve the same effect, if you feel this would make the body or role of
the new magic invoker function too convoluted.

One thing in its implementation too:

qDebug() << qPrintable(libraryName) << qPrintable(libraryVersion) << "DEBUG:"
<< msg;

the library name and version are in the ASCII subset anyway. qPrintable will be
a needless memory allocation and conversion, having no effect on the output. Or
does that somehow get rid of the QDebug behavior of putting "" around printed
QStrings? You can get rid of that by not making QStrings out of them in the
first place (also needless memory alloc and copy), but just using preprocessor
string concatenation like the old code did, as that yields a C string literal,
not a QString.

With these refactors, please verify the library still compiles and works fine
even if debugging is disabled at compile time (ENABLE_DEBUG_OUTPUT cmake
variable). And when it's enabled at compile time, that both runtime enable and
disable work, with the former producing sensible debug output with no
formatting issues like stray " characters.

You can verify the runtime debug disable behavior by adjusting
Test::initTestCaseImpl() in tests/lib/test.cpp, and running make check.

-- 
Configure bugmail: https://bugs.freedesktop.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the QA contact for the bug.