Before I give my review I want to point out that I have been using John's first logging library (with several key modifications) in production for a couple of years now. On the whole it has been quite a good library. Apart from allowing definition of logs in header files only I did not have too much difficulty massaging it to fit my needs. My approach to looking at this newer version of the library was to see if I could use it in a way that is similar to how I have used the first version. In addition I also wanted to see if it was possible to move past limitations I encountered in the first revision. Therefore with this review I'll point out what I tried to do with the library to put into context my later comments on it. Now for me I see logging as a very important library. There are plenty of options out there and many of us will have written our own at some point or other. With that in mind I feel that a logging library in boost is simply not about providing a library that allows logging. It is about tackling and solving the most critical aspects that any logging library will address, and providing a good interface on which to build more complex libraries related to logging. If a logging library is to be added to boost, then 'good enough' is simply not good enough. If I wanted 'good enough' I can write my own or choose from many of the libraries out there (including this one). So what are the minimum requirements? Well certainly there are a few and there is a good attempt at capturing them here <http://www.crystalclearsoftware.com/cgi-bin/boost_wiki/wiki.pl?Boost.Logging> For me though I wanted to be able to a couple of things and my efforts at using the library are focused on these. Essentially I wanted to be able to cheaply define logs in different scopes in what I would term library code. Therefore users of the library must be able to use the libraries without logging enabled and pay no cost. The fact that I have logging statements within my code should be transparent to them. At the log site I want to be able to write something like this (or even better, don't use a macro): BOOST_LOG(info) << "Some message" << std::endl; or perhaps BOOST_LOG(info,some_log_name) << "Some message" << std::endl; A number would also be sufficient instead of a level. This would be my default usage. Now I would like the client of the library to be able to enable and disable my logs in a simple fashion, for example using something like, boost::logging::enable_logs( "libraryx.*" ); Finally I'd like the client to specify what actually gets logged and in what format, things like, time level thread file line function That iwa my basic requirement. I tried to achieve this with this library. Now it is not actually possible to do that with this library in a convenient way because this version, unlike the last, does not support the notion of named logs in hierarchies. Having said that I had a go anyway to see how far I could get. I should also point out that I wanted as painless a way as possible of using logging (please see past my macro examples... that's what I started with in this library so it is hard move away from that) --- for adding log messages --- 1. #include <boost/logging/logging.hpp> 2. BOOST_DEFINE_LOG_IN_CURRENT_SCOPE or BOOST_DEFINE_NAMED_LOG_IN_CURRENT_SCOPE(some_name) 3. BOOST_LOG(level) << "message" << std::endl; --- for outputting logged messages --- 1. define a preferred format or simply take a good default 2. say where you want to log to 3. enable the logs you want --- I ended up writing a small file, <boost/logging/default.hpp> which basically provided wrapper macros for the ones that John has in his library. Here is what I put in it. ----------- #ifndef INCLUDED__boost__logging__default_HPP #define INCLUDED__boost__logging__default_HPP #include <boost/logging/format_fwd.hpp> #include <boost/logging/tag/high_precision_time.hpp> #include <boost/logging/format/formatter/tags.hpp> #include <boost/logging/format/formatter/high_precision_time.hpp> #include <boost/logging/format/formatter/named_spacer.hpp> namespace { typedef boost::logging::tag::holder < boost::logging::optimize::cache_string_one_str<>, boost::logging::tag::high_precision_utc_time, boost::logging::tag::level, boost::logging::tag::file_line, boost::logging::tag::function, boost::logging::tag::thread_id > default_tag_holder_string_t; } BOOST_LOG_FORMAT_MSG( default_tag_holder_string_t ) #define BOOST_LOG_DEFINE_LOGGER(name)\ BOOST_DEFINE_LOG(name,boost::logging::logger_format_write<>) #define BOOST_LOG_DEFINE_FILTER(name)\ BOOST_DEFINE_LOG_FILTER(name,boost::logging::level::holder) #define BOOST_LOG_ADD_DEFAULT_FORMAT_TO_SCOPE(logger_scope)\ logger_scope::logger()->writer().add_formatter\ ( boost::logging::formatter::named_spacer\ ("%time% %level% %file_line% {%function%} [%thread_id%] ")\ .add( "time",\ boost::logging::formatter::\ high_precision_utc_time\ ( "$yyyy-$MM-$dd $hh:$mm:$ss.$micro" ) )\ .add( "level",\ boost::logging::formatter::tag::level() )\ .add( "file_line",\ boost::logging::formatter::tag::file_line() )\ .add( "function",\ boost::logging::formatter::tag::function() )\ .add( "thread_id",\ boost::logging::formatter::thread_id() ) );\ logger_scope::logger()->writer().add_formatter\ ( boost::logging::formatter::append_newline_if_needed() ); #define BOOST_LOG_ADD_FILE_DESTINATION_TO_SCOPE(logger_scope,name)\ logger_scope::logger()->writer().add_destination\ ( boost::logging::destination::file( name ) );\ logger_scope::logger()->mark_as_initialized(); #define BOOST_LOG_ADD_DESTINATION_TO_SCOPE(logger_scope,type) \ logger_scope::logger()->writer().add_destination\ ( boost::logging::destination::type() );\ logger_scope::logger()->mark_as_initialized(); #define BOOST_LOG_DEFINE_LOG_IN_CURRENT_SCOPE\ static BOOST_LOG_DEFINE_LOGGER(logger)\ static BOOST_LOG_DEFINE_FILTER(filter) #define BOOST_LOG(the_level)\ BOOST_LOG_USE_LOG_IF_LEVEL\ ( logger(), filter(), the_level ) \ .set_tag( BOOST_LOG_TAG_LEVEL(the_level) ) \ .set_tag(BOOST_LOG_TAG_FILELINE) \ .set_tag(BOOST_LOG_TAG_FUNCTION) #endif // INCLUDED__boost__logging__default_HPP ----------- Ok, that's pretty hideous and I really don't like using macros, however it did allow me to add reasonable default logging. For example I can now write, --- #include <boost/logging/default.hpp> struct some_class { BOOST_LOG_DEFINE_LOG_IN_CURRENT_SCOPE inline void some_method() { BOOST_LOG(info) << "message" << std::endl; } }; --- To use the log I now write in some initialisation method elsewhere, --- void another_class:: initialise_logging() { boost::filesystem::path ExecutablePath( boost::filesystem::initial_path() ); boost::filesystem::path LogPath( ExecutablePath / "logfile.log" ); std::string LogFile( LogPath.string() ); BOOST_LOG_ADD_DEFAULT_FORMAT_TO_SCOPE (some_class) BOOST_LOG_ADD_FILE_DESTINATION_TO_SCOPE(some_class, LogFile); } --- For every log I want to use I have to add more entries, making this function very long for any remotely complex application. I am not advocating this. I'm merely pointing out that for the usage I was looking for, this library made it hard. In fact I wasn't able to use it as I had hoped and the only way I could get somewhat close was to write yet more macros. I believe this example is important because I see this as probably the most basic use of the logging library beyond simply writing to file directly (or std::cout or anywhere else for that matter). What I presented above is not acceptable, but I do believe the author has the ability to address these issues. In essence I think the above example should be very easy to write, with as few lines as possible and *only* use macros where absolutely necessary. It should be acceptable to use in library code with no impact on performance when logging is disabled. Advanced users with special requirements are then of course welcome to explore the customisation and extension points of the library. Some common customisations and extensions should be provided. Ok, with that context to my review behind us, I'll move onto the review proper. * What is your evaluation of the design? The design as it currently stands does not allow for the simple use case I outline. That does not mean I think it is failed but I do think it shows some important aspects of the design wanting. I do believe this can be addressed. --- As another reader noted concepts are not namespaces. I have no problem with grouping models of the same concept into the same namespace but that is not the same thing. Importantly concepts are named differently than namespaces. I encourage the author to look at how concepts are used in the standard and try to follow that convention. For example 'writer' is an ok name for a namespace but the concept is probably better named MessageWriter. I believe that the concepts used throughout the library could be better named to provide better clarity. I actually found the naming somewhat confusing at times. --- The omission of log hierarchies, or something that allowed the manipulation of several logs at one time, is an unfortunate omission and forced the library to be particularly heavyweight to use. --- Macros were relied on too much, and I realise the author has agreed to address this concern. --- I felt in general the organisation of the library was lacking and some of the defaults surprised me. If a library is to become part of boost one must expect it to work well with other parts of boost. - the default time handling should use the date_time library with easy selection between local time and UTC (which is often preferred) - threading should use the threading library, there should be no roll-your-own code in this important part of the library - the default path should be boost::filesystem::path --- Log Levels: Logging levels should be expressible as both numbers and names (maybe it is and I missed it). It should be possible to provide different name mappings easily (again maybe it is). The set of default levels should be more comprehensive to support a better out-of-box experience and the syslog levels should be part of that set. From <http://tools.ietf.org/html/rfc3164> Numerical Severity Code 0 Emergency: system is unusable 1 Alert: action must be taken immediately 2 Critical: critical conditions 3 Error: error conditions 4 Warning: warning conditions 5 Notice: normal but significant condition 6 Informational: informational messages 7 Debug: debug-level messages I'd suggest adding also, Fatal Severe Notable Verbose Detail Trace These could be ordered as: Fatal Emergency Alert Critical Severe Error Warning Notice Notable Informational Debug Verbose Detail Trace or some could simply be aliases for others. Regardless I shouldn't need to make modifications to use these common names. I'm not suggesting they all be used, simply that they are available. --- The rolling_log is not as I would expect. In most cases where I have used this I have a naming scheme that places the most recent log as the default log name and older logs should have numerically increasing names. Improving the rolling log behaviour should not pose any real difficulties. --- I got bitten by forgetting to use 'mark_as_initialised()'. This seemed symptomatic of the library as a whole. It was very easy to screw up. That implies to me that the interface was not clean enough, or more especially the default use relied on interfaces with too many variation points making it to easy to make a mistake. --- I am concerned about other aspects of the library, but most of these are covered in other reviews. Apologies for not recounting them here, but my time is short. I would say that I would like to see some co-operation between John and Andrey Semashev or at least some of the discussion from Andrey's review explored further particularly with regard to attributes. I am not saying I agree with Andrey, I am merely saying that it would be nice if some more concrete conclusion could be reached. I recognise also that there is likely no one best solution. * What is your evaluation of the implementation? The implementation needs tidied up. I realise this is a work in progress so that in itself is not a big criticism. The code is somewhat dense, hard to read, and the author seems averse to names which spell out what he means. I really see no need for short and cryptic names. Other reviewers have mentioned other things like line lengths and so on. As a side note I strongly encourage the author to install VirtualBox or similar on his Windows machine so he can install Linux as an alternative platform to try this on. * What is your evaluation of the documentation? The documentation has already been discussed at length regarding use of language structure and so on. I'd like to add my voice to those that wanted a clearer structure with more formal language. I'd also like to see that the documentation is easier to navigate. I saw no way to step through topics without having to go back to the main contents page. Some performance comparisons with other libraries may be in order. I am most interested in the case of disabled logging. Once I am actually logging something I am less concerned. At that time I care most about the information I log more than how quick it was (but of course quicker is better). One to three letter names in examples are not acceptable. What does g_l mean? I can *only* guess. * What is your evaluation of the potential usefulness of the library? This is a very important library of immense potential use. Moreover, if for some reason any future boost library itself was complex enough to warrant adding logging to, then this would be the library used. Of course that puts it into the same category as Boost.Test in terms of stability and core 'fit-for-purpose' needs. * Did you try to use the library? With what compiler? Did you have any problems? Yes, gcc 4.2.3, Debian Sid. I didn't encounter any real problems that haven't already been addressed in other reviews. * How much effort did you put into your evaluation? A glance? A quick reading? In-depth study? I added logging to a new project and used the log files produced to help track down a problem. I spent time trying to use the library and read the docs to help me with that. I spent a little time looking at the implementation to see if I could make small changes to enable capabilities I saw lacking. * Are you knowledgeable about the problem domain? Somewhat. * Do you think the library should be accepted as a Boost library? Be sure to say this explicitly so that your other comments don't obscure your overall opinion. I've been very encouraged by John's willingness to take on board suggestions and criticisms of his library. His clear enthusiasm to produce a great logging library is very evident. In many ways I'd like to vote yes and let John simply address all the concerns of the library (as he appears both capable of and willing to do). However I think this library is just too important for that to be acceptable. There are other efforts to produce a logging library for boost but so far only John has managed to get a library to review, twice. By and large both libraries have been good ones but both fell short for one reason and another. I think both John and the community has learned a lot from these reviews. I know I certainly learned of some use cases that I had never thought of. I do think John will be able to address most if not all of these with his library. What I would like to see, if possible, is for some of the current efforts on logging to learn from this review and either, join with John to help produce a better library, or get their library ready and into the review queue. As I said at the beginning I have been using John's previous library for some time now and with some success. This new library departed from the first revision perhaps too far for my liking, but once again John has encouraged a good discussion on the topic of a boost.logging library. This is all a very round-about way of saying that I vote Not To Accept this library into boost. But I would like to qualify that. I think the changes that John needs to make are too numerous to warrant a 'yes and tidy-up' vote. I believe there has to be another review. I would be very confident though in a positive outcome of such a review (from my perspective) if John succeeds in addressing the issues raised during review. This can only lead to a better library. I therefore encourage John not to wait too long before asking for his changes to be given a review. I'm sure it won't be a problem getting a review manager. Jamie