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.lu...@collabora.com _______________________________________________ LibreOffice mailing list LibreOffice@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/libreoffice
