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-11 20:34:19

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

> 
> Christopher Jefferson wrote:
>> 
>> On 11 Feb 2009, at 19:28, Andrey Semashev wrote:
>> 
>>> vicente.botet wrote:
>>>>> I have some innocent questions: * Is logging thread-safe? * If yes, 
>>>>> are the lines interleaved as it is the case for output streams? * If 
>>>>> not could you point out how and where in the implementation this is 
>>>>> handled?
>>>> Hi again,
>>>> well i have found some anwers to my questions on the document. I'll 
>>>> come back later on.
>>>
>>> I'm glad you did. :)
>>>
>>>> How a log record is recognized, i.e. I don't see std::endl neither 
>>>> std::flush are used in the examples. How many lines result in the 
>>>> following example if condifiton is true (2 or 3)
>>>>    src::logger_mt& lg = my_logger::get();
>>>>    if (lg.open_record()) {
>>>>        lg.strm() << "Hello ";
>>>>        lg.strm() << "world!";
>>>>    }

You have not answered my question. Anyway, looking on the code I've found that the log record delimiter is the destructor of the record_pump_type 
record_pump_type strm();

So when we write 

lg.strm() << "Hello ";

we have a record.

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

if (traces_enabled) {
    for (it=begin;it!end; ++it) { 
        std::cout << *it ;
        if (it+1!=end) std::cout << ", ";
        else std::cout << std::endl;
    }
}

So when I move to the logging library I would like to do something like.

if (lg.enabled()) {
    for (it=begin;it!end; ++it) { 
        lg.strm() << *it ;
        if (it+1!=end) lg.strm() << ", ";
        else lg.strm() << std::endl;
    }
}

But you force to create a temporary string stream which is in contradiction with your principles,i.e.  not simple neither efficient.

if (lg.open_record()) {
    string_stream strm;
    for (it=begin;it!end; ++it) { 
        strm << *it ;
        if (it+1!=end) strm << ", ";
    }
    lg.strm() << strm.str();
}

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.

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 << ", ";
    }
}

Best,
Vicente