Subject: [boost] [log] Boost.Log Formal Review
From: sam.gundry_at_[hidden]
Date: 2010-03-16 20:52:26
(This is my first Boost library review so take it easy...)
> Please explicitly state in your review whether the library should be accepted.
Yes. There are some concerns but I think it is very nearly complete, as far as my usage is concerned.
These concerns are elicited in detail below but for now my three major ones are:
- __LINE__, __FILE__ as attributes
- Boost.DateTime formatting is sloooowwww
- Ability to strip/compile out log messages in their entirety
> What is your evaluation of the design?
I haven't applied it heavily yet but so far the design seems logical. Separating the sinks and sources provides good flexibility. I'm yet to delve into usage of the filters though.
> What is your evaluation of the implementation?
I can't speak for the correctness of the implementation, but from a performance perspective, I've compared it to log4cxx (see the sourceforge thread Andrey has referred to) and it is comparable and/or better in most of my usages. Given the flexibility, I am prepared to pay for a little performance hits.
A concern, which again Andrey has mentioned during this review, is the speed of Boost.DateTime formatting. From our comparisons, it is about 3-5x slower than log4cxx. Is it possible to have a bare-minimum date time formatter similar to log4cxx's implemented? I wrote a quick little wrapper using a customised strftime (for microseconds) with the most basic caching (which log4cxx's formatter uses) and performance was comparable.
> What is your evaluation of the documentation?
For most the part, very good. Definitely sufficient/adequate for acceptance. The design and installation pages, etc, are extremely clear and easy to read. Personally, I would prefer a few more concrete examples. For example, (a minor one, I know) but how do you specify keywords via a settings file.
> What is your evaluation of the potential usefulness of the library?
High. An efficient, highly functional and configurable logging library has much potential.
I've found __LINE__ and __FILE__ missing as attributes. Andrey advised these are part of named_scope but the formatting is currently missing. Ideally - and I don't know if this is technically possible but - I would prefer these as just another attribute. Instead of requiring a BOOST_LOG_FUNCTION() call when you want to use them, you should be able to specify it as per usual, something a long the lines of:
backend->set_formatter( fmt::format(%1% [%2%:%3%] %4%)
% fmt::attr< SeverityLevel >("Severity")
% fmt::attr< fmt::scope::file >("File")
% fmt::attr< fmt::scope::line >("SrcLineNumber")
% fmt::message()
);
And then BOOST_LOG_SEV(...) << " a message" has the file and line attribute values already. Note, no need for BOOST_LOG_FUNCTION.
With respect to compiling out the log messages: it would be desirable for security, code protection and/or performance if the ability to specify certain log records to be stripped out of the source code.
A hack such as follows - which is obviously extremely inflexible - allows the users to define LOG_NTRACE to compile out all trace messages:
# if defined (LOG_NTRACE)
# define LOG_TRACE(logger, stream) (void(0)) ;
# else
# define LOG_TRACE(logger, stream) BOOST_LOG_SEV(logger, trace) << stream;
# endif
Something a lot smarter and flexible built into the library is desirable. Say, templated on your custom severity levels (or, indeed, any arbitrary attribute) and generating disable/enable macros.
> Did you try to use the library? With what compiler? Did you have any problems?
Yes. gcc 4.3.3, msvc 9, and qcc/gcc 4.x.x (can't remember) on QNX. No problems.
> How much effort did you put into your evaluation? A glance? A quick reading? In-depth study?
Initially, at the end of last year, I compared performance and functionality to log4cxx. However, for this review, I have had a quick read of the docs (not the code) and attempted integrating it for our purposes, about 4 hours worth.
> Are you knowledgeable about the problem domain?
Not hugely. Middle-of-the-road. Neither here nor there. 5/10. Relatively average.
A big thanks to Andrey for all his effort. Good luck with the acceptance!
Cheers,
Sam