Re: [boost] [log] review part 2

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

On 03/15/2010 11:21 PM, Steven Watanabe wrote:
>>> 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.
>
> You don't seem particularly shy about using macros in
> other places. This is a fairly standard use of macros
> in Boost.

I'd say the current usage of macros is quite moderate.

>>> 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.
>
> My comment here was actually tied to the one before. I'm
> willing to drop support for automatic invocation of Boost.Format,
> since it can always be handled explicitly.

I don't follow, could you explain? What exact usage of Boost.Format 
would you like to drop and why?

>>> 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.
>
> Given that LineID is one of the common attributes that
> you define, I don't see the point.

You have a point, will change the docs.

>>> Using scoped attributes to add attributes to a specific
>>> log record seems like a hack.
>>
>> Why?
>
> Because it doesn't match the intuitive idea of what
> you're trying to do.

It does. Scoped attributes mark records within the scope, no matter how 
wide the scope is - one record or 10.

>>> 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.
>
> http://www.boost.org/doc/html/proto/users_guide.html#boost_proto.users_guide.front_end.adding_members_by_extending_expressions

I've seen that. I can't remember the details now, I just remember that I 
stumbled upon something while following that approach.

>>> I wish that the names of functions and classes linked to the
>>> doxygen reference.
>>
>> Sure, if I figure out how to do it. :)
>
> I usually use something like [def __basic_core__ [classref
> boost::log::basic_core basic_core]]
> Also, in various places where you mention the examples directory, it
> would be nice if you
> could link to the source file for the specific example that you're
> referring to.

I was hoping there was some automated way to do it...

>>> 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.
>
> You have extract, attribute_value_extractor, static_type_dispatcher,
> and dynamic_type_dispatcher. AFAICT, attribute_value_extractor and
> static_type_dispatcher serve exactly the same purpose. I don't
> see a good reason to have both as public interfaces.

Not exactly. Type dispatchers, strictly speaking, are not tied to 
attributes at all. They are used to perform a double dispatch between 
the attribute value type and the visitor. They may well be used in 
another area (including, outside Boost.Log).

Extractors are more specific to the library and belong to a higher 
level. They accept the view of attribute values, and perform lookup of 
the attribute value. When it's found, type dispatchers are used to 
extract it and pass to the receiving function.

>>> 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?
>
> I don't think that it belongs anywhere. This is a general
> purpose utility that has no particular relation to logging
> and is not used by the library except in the tests. I
> don't think that using it is any better than

Sure you can write it yourself. But having that tool makes it easier to 
define simple handlers.

   make_exception_handler< runtime_error, exception >(
     var(cout) << &_1->*&exception::what);