Fwd: [ABANDONED] Remove some excessive log formatting

Stephan Bergmann sbergman at redhat.com
Fri Nov 22 13:02:16 UTC 2019


On 22/11/2019 12:13, Luboš Luňák wrote:
> On Thursday 21 of November 2019, Stephan Bergmann wrote:
>> * 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.

I meant the scenario where you enable one specific +INFO.area and the 
noise ratio for that area is high (like is the case for sal.osl, from my 
personal experience).

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

Assert is for programming errors, not for cases like the above where the 
8-bit pathname obtained from dladdr(3) may contain bytes incompatible 
with osl_getThreadTextEncoding().

(Problems with our defensive code base notwithstanding.)

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

That's why I wrote that I'm just "a little concerned"; nothing much to 
worry about :)



More information about the LibreOffice mailing list