Re: [boost] [log] review part 2

Subject: Re: [boost] [log] review part 2
From: Andrey Semashev (andrey.semashev_at_[hidden])
Date: 2010-03-15 12:34:31

On 03/15/2010 08:22 AM, Steven Watanabe wrote:
>
> I thought that the Boost.Parameter convention is
> to give keywords names starting with an _.

I think, some time ago there were two recommendations in the 
Boost.Parameter docs - the underscore and the namespace. Personally, I 
don't like leading underscores as it always makes me think of some 
implementation details.

> 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.

I thought if that. It was not done for two reasons:

1. The macro would have to have a formatter in its arguments. Given that 
the attribute definition should be exposed rather widely, this 
additional dependency would be unwelcome.
2. Guys that hate macros would start complaining.

> Since you allow use of Boost.Format, I don't think that
> the extra format argument of attr is necessary.

Boost.Format is currently invoked on strings, composed by attr 
formatter. Also, allowing to specify format even for streaming 
formatters looks like a nice feature.

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

This would require the attribute value to be copyable. Also, "extract" 
may accept a MPL sequence of types, in which case it would have to 
return something like optional< variant< types > >. This looks like an 
overkill.

The attribute value interface offers the "get" convenience method that 
returns optional. But, yes, you'd have to find the attribute value first 
in the view.

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

No. But it seems to be a reasonable improvement. Thanks.

> 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?

That should be ok.

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

The names may differ, the point is the same. I didn't try to force any 
names with my code samples. Users are free to decide which names fit 
them best.

> 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.

The time-based window has much more sense than a record count-based one, 
for the reason you have pointed out. If the application is actively 
emitting log records, the window gets bigger in order to accommodate 
more records that may go out of order. If records are emitted once in a 
while, the window is nearly empty all the time.

The time measurement also gives the idea of the sink minimum latency.

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

"Dec" stands for "decorator". But xml_escape is also fine.

> 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.

Yeah, that section needs updating. It's custom_severity_mapping, 
actually, and it really does support non-integral types.

> Some of the code examples overflow the window.

Which ones? Also, what resolution should I target? I'm using 1280x1024, 
and everything looks ok.

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

They are implemented with the common base class. Different top-level 
classes are defined in order to help with type safety. The 
custom_severity_mapping returns syslog::level_t, and mappings for event 
log return their corresponding types.

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

Why?

> 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.

I don't know about Phoenix 3, but when it's released, I'll take a look.
I considered and even tried to port on top of Proto, but dropped in the 
end for several reasons:

* I couldn't figure out how to make Proto expressions to be function 
objects.
* The resulting code looked heavier than my current solution.
* I didn't see much advantage in this port.

 > For instance satisfies, would be better
 > as bind(f, attr<...>(...));

I think, "satisfies" is more telling than the binding you suggest.

> 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.

There are several tools in the utility folder that could be extracted 
from the library. I once asked if there is any interest in it on this 
list and got no response.

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

Sure, if I figure out how to do it. :)

> 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.

I guess, I was inaccurate. It's non-copyable and non-assignable.

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

Right.

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

No reason in particular. This can be changed.

> 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.

What interfaces do you have in mind?
Type dispatchers are the lowest level tools. Attribute values 
dispatching builds on top of it. Your 'visit' seems something in between 
the type dispatchers and 'extract'. It so happened, I didn't need such 
middle-placed tool and thus didn't make it.

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

Huh? Where does it belong then? Or do you mean it could be extracted 
into a separate library?

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

Yes, but not in Boost releases the library is currently compatible with. 
If Boost.Log gets accepted, I will move to Boost.Xpressive, the code is 
there, but commented.

> 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);

Right, it needs updating.

> What happens if...
>
> A filter or formatter tries to log something.

Bad things may happen. The logging library should not log. Or at least, 
log so that no recursion occurs.