Re: [boost] [log] review part 2

Subject: Re: [boost] [log] review part 2
From: Steven Watanabe (watanabesj_at_[hidden])
Date: 2010-03-15 01:22:51

AMDG

I've finished reading through all the documentation
except for the reference.  Here are the comments
I have so far.  Overall the documentation is quite good.

I thought that the Boost.Parameter convention is
to give keywords names starting with an _.

I think that it would be a little nicer
to work with attributes with an interface
like this:

BOOST_LOG_ATTRIBUTE(logging::trivial::severity_level, severity);
BOOST_LOG_ATTRIBUTE(unsigned int, line_id);

fmt::stream
    << fmt::line_id()
    << ": <" << fmt::severity()
    << "> " << fmt::message()

That way, it's easier to guarantee static type safety and
is also a little less verbose when accessing the attributes.
Since you allow use of Boost.Format, I don't think that
the extra format argument of attr is necessary.

I don't like the inconsistency of
"sample_%N.log",
vs. "[%TimeStamp%]: %_%"
Could you use "sample_%N%.log"?
Nevermind - I see that you're matching Boost.DateTime

I would like logging::extract< unsigned int > to return
a boost::optional or something like that instead of having
an output parameter.

Is it possible to declare and define a global logger separately?

I think LineID may be a bad name, since when I saw it, I assumed
it meant the line in the source that generated the log message,
perhaps RecordID would be better?

I don't like the inconsistency of
logging::make_attr_ordering< unsigned int >("Line #").
I though the name was LineID?

I don't like the use of a time in ordering_asynchronous_sink
to specify how long to wait for a new record.  It just seems
too fragile.  Suppose that the machine is running under a heavy
load or I suspend the process, or put the machine to sleep?
I don't like having the correctness depend on how long things
take.  If nothing else, it needs to be stated that
ordering_asynchronous_sink is only a "best effort" mechanism
and makes no guarantees.

fmt::xml_dec.  I would prefer a more intuitive name like
xml_encode or xml_escape.  (Since dec is "obviously" an
abbreviation of decrement)

In 
http://boost-log.sourceforge.net/libs/log/doc/html/log/detailed/sink_backends.html,
the documentation says that custom_level_mapping works for
integral types, but the example uses std::string.

Some of the code examples overflow the window.

Why are there separate classes for handling the syslog and
windows event log mappings?  It seems like they're basically
doing the same thing.

Using scoped attributes to add attributes to a specific
log record seems like a hack.

I don't really like to see the lambda capabilities implemented
from scratch.  At the very least, I'd like to see plans for
porting it to Phoenix v3 once it's ready.  A full lambda library
is far more feature complete than the minimal lambda that
you provide.  For instance satisfies, would be better
as bind(f, attr<...>(...));

For string_literal, I seem to recall that Boost.Test already
has a basic_cstring.  Maybe this should be factored out
instead of being duplicated.

I wish that the names of functions and classes linked to the
doxygen reference.

You claim in
http://boost-log.sourceforge.net/libs/log/doc/html/log/detailed/utilities.html
that std::type_info is required to be unique for each
type.  I could not find such a requirement in the standard.
Please provide the reference.

Also, std::type_info has no member raw_name in the standard.
If it exists, it's an extension.

Why is type visitor::visit not spelled operator()?

The stuff for extracting the values of attributes is so
complex.  Why can't you just have a single interface like
visit<boost::mpl::vector<int, char, double> >(attr, f);
rather than providing all these complicated layers.
I guess that doesn't work when the set of possible types
isn't known at compile time, but I still think it should
be possible to cut down the number of redundant public
interfaces.

I'm not convinced that make_exception_handler belongs in
this library.

In the todo section I noticed a reference to #3278.  This
was fixed a while ago.

In
http://boost-log.sourceforge.net/libs/log/doc/html/log/extension.html
under the minimalistic sink backend, the description of
consume doesn't seem to match the code:
  "The record, as it was stated before, contains a set of attribute
  values (goes as the first argument of the function) and the message
  string (goes as the second one)."
vs.
  void stat_collector::consume(record_type const& rec);

What happens if...

A filter or formatter tries to log something.

In Christ,
Steven Watanabe