[boost] [log] Comments

Subject: [boost] [log] Comments
From: Vladimir Prus (ghost_at_[hidden])
Date: 2010-03-15 06:45:12

Hello,

I have a few comments and questions about the Boost.Log library currently
being reviewed. This email is not actually a review as such.

It will help if I explain where I'm coming from. I am most familiar with
two logging mechanism. One is from KDE libraries. There, you have severities
and component ids -- which are just integers. So, source code might have

        kDebug(9012) << "doing something";

There's GUI to configure which components have logging enabled, and there's
a way to omit explict component id. Another logging mechanism is from
Eclipse. There, you specify severity and component id -- this time a string.

In KDE, output goes to a console. In Eclipse -- to a log file. I would
those mechanism to be sufficient for all my needs.

Looking at the proposed libraries, I see two major issues:

* There's no default setup that is comparable with the two mechanisms above.
BOOST_LOG_TRIVIAL has only severity and it appears adding component information
is complicated. In a sense, it looks like the library provides building blocks,
but not a solution I can immediately use.

* I think the library elegantly uses various facilities like Boost.Parameter and
lambda expressions to provide an easy interface. Unfortunately, it seems like it's
failing to provide any other interface for folks who might be unhappy with lambdas
and named parameters.

There's a more detailed explanation of those major issues:

* It appears that out of the box, the proposed library does not address
this component/severity style of logging that I've explained above. The
proposed solutions at:

        http://permalink.gmane.org/gmane.comp.lib.boost.devel/200793

sound relatively complex.

* The example of specifying filter without lambda functions given
in http://permalink.gmane.org/gmane.comp.lib.boost.devel/200793
is frankly scary. I believe that in most languages and environments
I worked with, lambda functions are just convenient alternative to
defining the same function explicitly. Why is in Boost.Log, 
lambda expressions are blessed with extra convenience?

* Why can't I use the library without named parameters? Surely,
there are low-tech ways to specify things.

* Why does channel_logger has a *single* parameter, and still requires
that you pass it via named parameter. That is:

            src::channel_logger<> cl("foobar");

     produces incomprehensible error message.

Other concerns:

* The library went too far with namespaces. Why are *six* nested namespaces
necessary? This appears to be just asking for extra typing for no good
reason.

* Why is trivial logging printing the sequental number of each log message?
I don't think this helps anything. Also, it prints the thread "number",
on my system a hex string. I am not sure thread id should be part of trivial
logging. Finally, I think by default, everybody things of logging as glorified
std::cout, so trivial logging better print messages to console, not to a file.

* Why does the library have light_rw_mutex? I would really prefer if everything
thread-related be located inside Boost.Thread, so that at least the code can
be readily audited.

* It appears that the default behaviour of fmt::attr is to throw is the attribute
with such name is not found. This seems like a bad choice to me. A logging
library should never fail -- it is the last resort at diagnosing a problem in the field,
and if it throws by default, it's useless. 

* I have used the attached script to measure code size impact. The script basically
creates a test file with a single function calling BOOST_LOG_TRIVIAL and measures
the size of that function depending on the number of calls. I've got 139 bytes per
call for 'g++ -Os' and 174 per call for 'g++ -O3', which seems nice. This is function
code only. When counting file size I get 324 and 514 respectively. For reference, kDebug
gives 136 and 328 of file_size_bytes per call. I guess these results are OK.

* I see lots of warnings in slim_string.cpp, e.g:

        ../../../../libs/log/src/slim_string.cpp:237: warning: control reaches end of non-void function 

I'd recommend working with maintainer of boost/throw_exception.hpp to add noreturn attributes on
gcc, and silence the warning in other ways on other compilers.

* Building the library with threading=single results in unresolved references to pthread functions.
I belive that library should not use any sync mechanisms in this case.

Thanks,
Volodya