Re: [boost] [log] Review-ready version in the Vault

Subject: Re: [boost] [log] Review-ready version in the Vault
From: vicente.botet (vicente.botet_at_[hidden])
Date: 2009-02-12 12:47:59

----- Original Message ----- 
From: "Andrey Semashev" <andrey.semashev_at_[hidden]>
To: <boost_at_[hidden]>
Sent: Thursday, February 12, 2009 5:55 PM
Subject: Re: [boost] [log] Review-ready version in the Vault

> 
> vicente.botet wrote:
>>>>> That one is not valid in the current implementation. It should be 
>>>>> replaced with:
>>>>>
>>>>>  if (lg.open_record())
>>>>>    lg.strm() << "Hello ";
>>>>>  if (lg.open_record())
>>>>>    lg.strm() << "world!";
>>>> What would happen if someone did write the above?
>>> Officially - unspecified behavior. In current implementation, in case if 
>>> no records are open recursively, the library would open a new record itself.
>> 
>> Unacceptable IMHO. The user don't need to check if logs are enabled. 
> 
> He does need to open a record in some way. Opening a record is not only 
> filtering, although this is a part of the process. The key point of 
> opening a record is acquiring values of all attributes and composing 
> them into a single view. It is this view then processed by filters, 
> formatters and sinks.

I have no problem with opening the log record, the problem is that you close the record as soon as the destructor on the remporary is called.

Anyway, this is an implementation detail. Checking should be be able to be done without a log record opened.

>> I don't like that 
>>     bool open_record();
>> open a record and check if the logging is enabled and the record is closed only when a temporary record_pump_type is destroyed.
>> 
>> Why not have 3 functions enabled, open_record and close_record, and why not have a scoped_record?
> 
> For the reason I've described above. "enabled" will have to implicitly 
> call "open_record".

I desagree in this pooint.
 
> > and why not have a scoped_record?
> 
> That's another question. The main background behind the current design 
> decision is that the scoped_record object introduces a danger of misuse. 
> For example, one could attempt to pass it to another thread or store in 
> a container. 

Well, you can delete the operator&, copy constructor, copy assignement, ...

> On the other hand, I realize that the current interface is 
> not perfect, too, so I have no strong position. FWIW, it should be quite 
> simple to change this.

I'm sure that this will not be complicated. But this can not be made from outside, so I need you change it.
 
> > Well the open_record will be not needed if we have always a record
> > opened (i.e. a new record is opened implicitly after a close_record.
> 
> That is not possible for several reasons:
> * To construct the attribute values view the set of the source-specific 
> attributes is needed
> * This view will not be able to reflect changes to the attribute sets
> * This introduces problems in case of recursive logging. E.g.:
> 
>   int foo()
>   {
>     BOOST_LOG(lg) << "hello";
>   }
> 
>   BOOST_LOG(lg) << foo();

Ok, I see. 
 
>>>> I really don't think this is an acceptable limitation. Don't you think 
>>>> people will often write to a stream more than once in the same function?
>>> No, I think they won't. Most of the time BOOST_LOG & co. macros will be 
>>> used, and they hide this "if" internally:
>>>
>>>   BOOST_LOG(lg) << "Hello";
>> 
>> Sorry, but people has a lot of different needs. When the log library use the << operator, he must mimic the standard STL. The user needs to master the log record delimiter in some way. Some times we need to use some conditions and loops to prepare the log record. We can found code like the following :
> 
> [snip]
> 
> All these cases are covered by stream manipulators and operator<< 
> overriding. I must say, this approach is much more appealing than 
> writing inline loops etc., since it may be reused regardless of logging.

This was just an example. You will need to inline loops on the  operator<< overriding, isn't it?
 
>> What do you think about this 
>> if (lg.enabled()) {
>>     for (it=begin;it!end; ++it) { 
>>         lg << *it ;
>>         if (it+1!=end) lg << ", ";
>>         else lg << std::endl;
>>     }
>> }
>> 
>> std::endl will close the record.
> 
> I would really dislike to override behavior of the standard 
> manipulators. 

Why? Is there a deep reason?

> And introducing a special manipulator doesn't seem 
> reasonable to me.

Neither to me.
 
>> Or with a scoped log record
>> if (lg.enabled()) {
>>     scoped_log_record rec(lg);
>>     for (it=begin;it!end; ++it) { 
>>         rec << *it ;
>>         if (it+1!=end) rec << ", ";
>>     }
>> }
> 
> That looks much more attractive. However, I had something like this in mind:
> 
>   if (log_record rec = lg.open_record())
>   {
>     rec.strm() << ...;
>   }
> 
> The one problem I see here is how to deduce the log_record type from lg. 
> I don't want to use Boost.TypeOf since it looks too heavy to me.

What about defining it inside the logger?

    logger_type lg;
    if (logger_type::scoped_log_record rec = lg.open_record())
    {
        rec.strm() << ...;
    }

Why do you need to get the strm to be able to log? Why not just

        rec << ...;

Best,
Vicente