On 21/11/2019 12:47, Luboš Luňák wrote:
Hm, sounds rather like a misuse of the SAL log facility to me. (Each
use of it comes at a cost, so it shouldn't be used too lightly for
anything but logging unusual events.)
E.g. currently while writing the Skia VCL backend I've added SAL_INFO to
pretty much to each of the drawing functions. And of course normally nobody
wants to see that, but it can be useful if needed, all it takes is setting
SAL_LOG, and it fits nicely with the code (no #ifdefs or anything). So I
disagree with it being just for the unusual stuff (that's SAL_WARN).
Is it really that costly? It's no-op for non-debug builds, and I was under
the impression that even for debug builds it tries to be fast in the common
case when it's a no-op. So I'd expect this to be insignificant, especially
when compared to costs such as using debug mode libstdc++.
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.
* 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.
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.
* Use SAL_INFO if it is an unusual event that is not obviously an error.
Like
SAL_INFO_IF(
pLib == nullptr, "sal.osl",
"dlopen(" << pModuleName << ", " << rtld_mode << "): "
<< dlerror());
in osl_loadModuleAscii (sal/osl/unx/module.cxx), where only the calling
code will know whether or not it is in an error if the requested module
does not exist. (And where the captured dlerror() output is information
that (a) is not available to the caller, and (b) may be valuable in
cases where the caller decides that the event is an error.)
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.
_______________________________________________
LibreOffice mailing list
LibreOffice@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/libreoffice
