[boost] [log] Boost.Log formal review

Subject: [boost] [log] Boost.Log formal review
From: barend (barend_at_[hidden])
Date: 2010-03-16 15:39:56

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