Re: Fwd: [ABANDONED] Remove some excessive log formatting

On Thursday 21 of November 2019, Stephan Bergmann wrote:
> My concerns are:
>
> * We may eventually want to enable SAL_WARN/SAL_INFO for production
> builds, at which point the space cost of log message strings might
> become an issue.

 Space cost? The size of /usr/lib64/libreoffice/program/*.so here is about 
200MiB. Even if we had 2MiB of log messages code, which would be probably an 
insane amount, that'd be still a negligible 1%.

> * Too much trivial SAL_INFO noise (that was once added ad-hoc and should
> arguably have only been in a developer's local build) can make it
> unnecessarily hard to see the more relevant SAL_INFOs (and log area
> granularity is limited in practice).  Of course, it is hard to tell how
> much of the existing SAL_INFO use falls under that noise category.

 If I'm not mistaken, all SAL_INFO is disabled by default, which trivially 
solves that. In case you meant SAL_WARN, that indeed can be a problem, but 
rather a problem of either there being too many warnings that nobody cares 
about, or those warnings not actually being important enough to be warnings.

> When I had originally designed SAL_WARN/SAL_INFO, I had anticipated use
> along the lines of:
>
> * Use SAL_WARN if it is an obviously erroneous unusual event.  Like
>
> >             SAL_WARN_IF(
> >                 *ppLibraryUrl == nullptr, "sal.osl", "rtl_string2UString
> > failed");
>
> in osl_getModuleURLFromAddress (sal/osl/unx/module.cxx), where it warns
> if the passed-in ppLibraryUrl cannot be converted to an 8-bit string.

 I think that if something is an obvious error, then the code should just 
plain assert. IMO the codebase is way too defensive and it's one of the 
reasons why there are so many warnings that nobody cares about - because they 
do not have to.

> I do not claim that this is the only reasonable approach to using
> SAL_WARN/SAL_INFO, and I have seen more liberal use effectively from day
> one.  But I'm always a little concerned, for the reasons outlined above,
> when I come across what looks like overly liberal use of SAL_INFO.

 One way to address your concerns might be by adding SAL_TRACE (or whatever 
it'd be named), which would be even one level lower than SAL_INFO and it 
wouldn't even be compiled in release builds if we get to the point of keeping 
SAL_INFO for those. But when we got to looking at the actual issue, how this 
would actually matter. The purpose of keeping SAL_INFO in release builds 
would be presumably that users could provide such output when asked for e.g. 
in bugzilla, but then how to decide where to draw the line? If I want the 
debug output, I probably want all of it.

 And of course all of this matters only if it actually matters in practice. 
I'm still fairly convinced that both the space and time cost can be 
negligible, you have designed it to be pretty cheap if not used (with the 
possible exception of sal_detail_log_report(), but that could be improved if 
needed).

-- 
 Luboš Luňák
 l.lunak@collabora.com