Re: [Libreoffice] Assertions and Logging

On 11/22/2011 05:17 PM, Lubos Lunak wrote:
>   First of all: I see this has been already committed, after giving people only
> slightly more than the weekend and the day after it to react, which I
> consider too short for something with as large scope as this. Some of us
> don't work on the weekends and have a backlog when they come back on Monday.

I did not commit it in order to stop any discussion.  Sorry if it looked 
that way.  Rather, as I did not get any totally disagreeing reactions, I 
thought it would be easier to polish this further if the code is 
actually in.  (And I think it was a good decision to commit it early; 
naturally, the various tinderboxes pointed me to some flaws that were 
still in.)

> On Friday 18 of November 2011, Stephan Bergmann wrote:
> ...
> > - The implementation is somewhat careful to produce as little code as
> > possible at the call-site of the new macros, and to keep the code path
> > for suppressed logs as short as possible.  However, the C++-stream-style
> > macros will potentially always be more expensive than the
> > C-printf-format-style ones.
>
>   Does it really matter, when it is used only in debug builds anyway? Debug
> builds are slower already anyway, and if the cost of generating the output is
> considered expensive, what about the cost of wherever the output will end up?

While the macros are currently only enabled in debug builds, I see no 
reason that should always be so.  A production build from which one can 
extract such logging information would be quite useful, I think.  So I 
designed the macros with that in mind.

> > One open question is which set of macros (the C-printf-format-style ones
> > or the C++-stream-style ones) to give the "natural" names without
> > additional suffix.
>
>   IMO that is a rather simpler decision: The natural names should be used by
> the only set that exists, for a number of reasons:
> - the C variants don't work with C++ strings (or other non-trivial data),
> which makes them next to useless
> - this is debug code, so the slight performance cost is negligible

See above.  I think its important to keep the most common case (the log 
message can be expressed as a static C literal) cheap.

> - where do we have non-C++ code that has actually needs to use this
> functionality and cannot just have the filename extension changed?

Of course, if "works in C" were the only reason for the non-S variant, 
we could change existing C files into C++ ones as a prerequisite for 
adapting them to sal/log.h.  (And changing them is a useful exercise 
regardless, anyway.)

Maybe it would make sense to simplify the non-S variants so that they 
only work with a plain string, removing the printf-style format feature. 
 But that would mean some work, adapting the existing uses of OSL_TRACE 
that depend on that feature.

> - people will mix the two sets randomly, with all the consequences (mess,
> wondering when to use which one)
> - the _S suffix is ugly and it is the variant that'll eventually be used more
> - if there are any (real) problems with the C++ variant, perhaps they should
> be simply rather fixed/improved

The only problem with the S variants is the call-site bloat of creating 
a std::ostringstream and piping into it, etc.  I don't see how that can 
elegantly be avoided, though.

> - WTH should we intentionally have two sets of one functionality again?

Yeah, I get your point.

Stephan