[boost] [log] Boost.Log formal review result

Subject: [boost] [log] Boost.Log formal review result
From: Vladimir Prus (ghost_at_[hidden])
Date: 2010-03-24 18:28:30

First of all, I would like to thank Andrey for this submission and the
active discussion during the review. I also would like to thank everybody 
who participated in the review.

The comments on the library were generally positive. Several reviewers
have tried it in practical projects already, and found it superior to
other libraries. It clearly will benefit the users of Boost right away.
Therefore, the library is accepted. Congratulations, Andrey!

On the other hand, some concerns about the ease of use and performance
and implementation details were raised. Of those, some can be easily
addressed as part of normal maintenance, while some seem to require 
significant rework and may change the library quite consirably. Therefore,
the library is accepted subject to several conditions -- listed below
under "Critical issues". Because the changes can be large, I think a
mini-require is required when those issues are addressed. To avoid delays, 
it's probably not necessary to formally schedule and conduct a mini-review --
just posting a email asking for discussion will be sufficient.

Statistics
==========

There were roughly 300 emails during formal review (I have a mbox archive
if anybody needs it). 

The following persons submitted a review with a vote. Some of votes are
"provisional", this is not reflected below, but affects the list of
critical issues below.

- Boris Schaeling (yes)
- Roland Bock (yes)
- Ralf Globisch (yes)
- Sean Chittenden (yes)
- Darryl Green (no)
- Tom Brinkman (no)
- Barend (yes)
- Sam Gundry (yes)
- Vicente Botet (yes)
- Robert Stewart (yes)
- Alexander Arhipenko (yes)
- "M-Square" (yes)

Critical issues
===============

These issues must be addressed, and be passed through informal mini-review,
before the library can be added to SVN.

- It appears that the 'trivial' logging is not sufficiently good to just
  take and use. The library should provide an out-of-the-box mechanism that:

  - supports channel/severity attributes, without creating new logger for
    each severity. It also should be possible to filter out specific
    channel by essentially poking at some "map", and without manually
    composing a new filter that checks for a set of allowed channels
    sequentally. I believe that at least Christian, Roland, Robert and myself
    requested this. 

  - has no observable effect on compile time. One second (or more) that trivial.hpp 
    imposes is not acceptable.

  - outputs to console by default, but permits changing that

  - outputs only component/severity/message by default, but probably permits
    changing that.
  
- By default, the library should try hard to continue working no
  matter what errors happen during processign a log record -- including
  things like invalid attribute names or types, or other exceptions.

- The library should not reinvent wheels. In particular, custom implementations
  of TSS and RW mutex seems like a very bad idea. Use of a custom lambda
  implementation was pointed by many as not great -- we already have a couple,
  so let's not add more to the mix.

- Library interface is too geared towards lambda. In particular, implementing
  formatters/filters as free-standing functions appears to require considerably
  more code. In particular, it should be possible to access an attribute using a
  single function call. Also, logging::extract taking a lambda for callback
  complicates matters, it's desirable to have a more direct variant.
  

Important issues
================

These issues are important enough to be mentioned here, but can be
addressed as convenient.

- The reported run-time performance can be improved. In particular the 
  performance with date attributes seems scary and 2x slowdown relative
  to some other library in the 'most records are filtered out' case is
  also concerning.

- I don't think it was satisfactory explained why 'attr' exists in two
  namespaces, as opposed to being a single type that is used for both
  formatting and filtering.

Other notes
===========

These issues seem important and should be considered, but might not even
be fixable within the current design.

- Several reviewers said the library is "too flexible" and this makes it
  harder to understand. Probably nothing can be done about that at this
  point, but interesting data point notwithstanding.

- It was suggested that the library include an API that another Boost log
  may use in a way that permits plugging another logging library should
  end user of a Boost so desire. This seems a good idea, but I think it's
  up to Andrey to implement such API or not.

- Alternative design approaches were proposed, in particular (i) permitting
  user log records and templating/basing other components on a user-defined
  record type and (ii) using compile-time indexing to access attributes.
  Also, it was proposed that named parameters not be necessary to use the
  library, and a more conventional interface be available.
  It does not seem that implementing those suggestions is fully possible
  without writing a different library from scratch, so it's up to Andrey
  to consider those proposals.

- It was suggested to use newer Spirit to address poor compile times for
  the library itself.

Thanks,
Volodya