Re: [boost] [log] review part 2

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

AMDG

AMDG

Andrey Semashev wrote:
> On 03/15/2010 11:21 PM, Steven Watanabe wrote:
>>>> BOOST_LOG_ATTRIBUTE(logging::trivial::severity_level, severity);
>>>> BOOST_LOG_ATTRIBUTE(unsigned int, line_id);
>>>
>>> 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.

Why is it necessary to pass a formatter to the macro?

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

I'm just pointing out the the arguing that this solution is
bad just because it uses macros, leads nowhere.

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

attr<unsigned int>("LineID", format="%08x");
It's not that necessarily I want to drop this, but that
I don't see it as an important feature.

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

No it doesn't.  It is true that setting an attribute for one record is a
special case of setting it for a scope, but it is a long way from
being the first thing I would think of.

In fact, the semantics are not what I would expect
when I just want to pass a parameter, according to
http://boost-log.sourceforge.net/libs/log/doc/html/log/rationale/why_weak_scoped_attributes.html

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

You're not writing a general purpose dynamic dispatching library,
you're writing a logging library.  If I wanted such a utility I would
certainly not look for it in Boost.Log.

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

var(std::cout) << bind(&boost::current_exception_diagnostic_information)

In Christ,
Steven Watanabe