Hi, This is my review of Boost.Log submitted by Andrey Semashev.
Please explicitly state in your review whether the library should be accepted.
I've been doubting about this for a few days. When I started my review, by trying to use the library, I discovered that the library is not header-only. I had thought that a logging library should be lightweight and had therefore expected only headerfiles. I then tried the library from John Torjo from 2007 but it is not header- only neither, so no reason to favour that one (I didn't dive into it further). Because of some messages on the list, a.o.:
I think that any reasonable log library must be compiled to it's own lib due to the intermodule singleton requirement
I realized that it is indeed useful to have a non-header-only option as well. The answers of Andrey were satisfactory, a.o.:
If we have a list of places in Boost where logging may be applied, perhaps we could compile a set of requirements for such a lightweight wrapper. When we have it, it would be easier to tell, whether Boost.Log fits the role or not, or whether the wrapper should be part of it or a separate submission. I'd be interested to hear from the library authors on this topic.
And I found in the Boost.Log documentation, TODO-list:
Think over a header-only configuration. Perhaps, with a reduced functionality.
So, good. Then I started the real review and I started to like the library, it is definitely useful, is configurable, trivial logging is quite simple. It also contains very useful non-trivial features such as writing to the Windows Event Log and alarm critical states as balloon tips. So I finally decided to give my YES vote, contingent on the condition that a lightweight header-only configuration, e.g. single-threaded, single-module, will be available for library writers. Some more about this below. ---------------------------------------------------------------------
What is your evaluation of the design?
I like the options, the trivial logging, multiple back-ends, modularity and extensibility. I didn't have a detailed look to the design but in general it looks good to me.
What is your evaluation of the implementation?
I didn't have a detailed look either. I glanced through the sources and the implementation is looking neat. I wonder why the class "basic_slim_string" is necessary, to me it sounds very strange why a task as logging needs an own string implementation. However, I didn't have the time to dive into it and didn't ask questions on this on the list.
What is your evaluation of the documentation?
The documentation is good. Pages like design overview, tutorial, installation are easy to follow. The tutorial pages however contain some omissions and you have to refer to the examples to see what is missing. I mean here the chapter "Trivial logging with filters" which seems to give a complete example, but misses the namespace aliases, two of them would have been enough.
What is your evaluation of the potential usefulness of the library?
Very useful, definitely, for applications and for libraries.
Did you try to use the library? With what compiler? Did you have any problems?
Yes, I used it using Visual C++ 2008 Express Edition. Because Boost.Log is not header-only, and I don't have the Boost libs compiled on my machine (don't ask me why, I never have them), I had several issues which other reviewers probably didn't run into. The defines _CRT_SECURE_NO_WARNINGS and _SCL_SECURE_NO_WARNINGS were necessary to surpress M$ warnings. The define BOOST_ALL_NO_LIB is of course necessary to build it like I did, and it works for Boost.Log. The compiler complained about a missing simple_event_log.h by event_log_backend.cpp, so I drafted it by hand, but I later saw that it is documented and you should neatly use the message compiler, so OK. After adding some Boost source files from thread, filesystem, regex and system it worked, datetime was not necessary. A complete build (including all mentioned sources) costs about 2 minutes. The tutorial did work (see message above on filtering, adding namespace aliasses was necessary) and I experimented with some things, many options seems very useful to me such as the log format, automatic date time, log file rotation, log file max size, etc. etc. I like the streaming feature (so for me printf is not necessary). I have some reservations about the compiler support: GCC 3.4.5, the default on Linux and MinGW, is not supported and for a task as logging this seems to me as a disadvantage.
How much effort did you put into your evaluation? A glance? A quick reading? In-depth study?
About five to six hours spread over some days.
Are you knowledgeable about the problem domain?
Yes, I always need logging. In our library at Geodan we built logging as a singleton in 1995. We rewrote it to a logging-over-DLL's in 2000, but these two were not 10% as sophisticated and complete as this logging library is. So yes, these things were not header-only neither. --------------------------------------------------------------------- So back on the logging for libraries. If Boost libraries which are header-only will start to use this library, without protection, thousands and thousands of project files and makefiles worldwide will be broken. This answer:
If an existing header-only library starts using the Boost.Log library we'll need to update the makefiles. This is a reasonable cost
(not from Andrey)is therefore not satisfactory to me. This answer:
Which probably means that the decision is with the author(s) of each individual library.
(from the review manager) surprised me a bit, I think that in a library under review such an important thing may certainly be discussed, per library. However, as said, it is not a reason to vote no, because the rest of the library is quite good. A small macro (sorry) wrapper would already do most of the trick, and avoid breaking project files: #if defined(BOOST_ALL_LOG) || defined(BOOST_GEOMETRY_LOG) #define BOOST_LOG_GEOMETRY(o, s) BOOST_LOG_TRIVIAL(o) << s #else #define BOOST_LOG_GEOMETRY(o, s) #endif Where it can be called in libraries as: BOOST_LOG_GEOMETRY(info, "Log line with a value " << 3); This is inspired on the general define "BOOST_ALL_NO_LIB" and library- specific define "BOOST_LOG_NO_LIB". This small wrapper would really be useful, at least for us, and replace the #ifdef BOOST_GEOMETRY_DEBUG lines which are currently in our Boost.Geometry library. This is zero cost when one of the macro's BOOST_ALL_LOG or BOOST_GEOMETRY_LOG is not defined (by default it is not), and contains much functionality of Boost.Log as filtering, rotation, etc, when one of these macros is defined. Of course it can be enhanced or there are better solutions. Anyway, a lightweight solution will not be difficult. --------------------------------------------------------------------- So thanks Andrey for submitting this library, thanks Volodya for managing the review, and I hope it will be accepted with a lightweight header-only configuration included. Regards, Barend