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

Subject: Re: [boost] [log] Review-ready version in the Vault
From: Andrey Semashev (andrey.semashev_at_[hidden])
Date: 2009-02-12 11:55:17

vicente.botet wrote:
> 
> You have not answered my question.

Actually, I did write in my reply to you nearly the same that you've 
discovered. Doesn't matter, though.

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

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

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

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

> 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. And introducing a special manipulator doesn't seem 
reasonable 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.