(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