[Libreoffice] OSL_ASSERT - remove backtrace or make abort
l.lunak at suse.cz
Thu Apr 21 08:12:54 PDT 2011
On Wednesday 20 of April 2011, Thorsten Behrens wrote:
> Lubos Lunak wrote:
> > I'd like to remove the backtrace printing from OSL_ASSERT and friends,
> > or, even better and if possible, make these functions work properly, i.e.
> > abort on failure (I'm not really holding my breath on the second one, but
> > refusing that one will at least support the first one).
> Yes - first of all, there's SAL_DIAGNOSE_ABORT, to optionally enable
> your desired behaviour.
I know, but that's useless in practice. If the usual approach is to treat
OSL_ASSERT as non-fatal, then nobody will enable SAL_DIAGNOSE_ABORT, as that
wouldn't get them very far. It makes more sense the other way around, the
default being fatal and something like SAL_DIAGNOSE_DONT_ABORT allowing for
an override in special cases.
> Then, there's sal-disable-backtrace.diff,
> which I can happily merge - just set DISABLE_SAL_BACKTRACE then.
Again, it's more about the default being wrong, as in the usual case the
current default is about spamming the output with almost useless backtraces.
Ok to use the attached patch instead?
> Regarding the problem itself, it's festering since many years, and
> not easily reconcilable - in the sal/uno area, assertions *are*
> serious, and should lead to aborts. Especially in the
> application/filter area, though, those were indeed often used in a
> "um, not sure, looks fishy here, let's do something"-kinda way.
> Generally, cleaning that up (and converting the mis-used assertions
> I mentioned into some warning) would be greatly appreciated.
> Something for past-3-4 and a feature branch?
One thing worth noting is that the current behavior appears to be
intentional: OSL_ASSERT stuff is documented and it says
e.g. "OSL_ASSERT(cond) If cond is false, reports an error.", which I don't
read as "and aborts". However, given how confusing the usage currently is in
same places, I'd expect this not to be a bigger problem than some other way
of cleaning this up.
So, as the plan to handle this I'd suggest that we change the docs to say
that these are now fatal and that non-fatal checks should use newly
introduced OSL_WARNING(). Non-fatal OSL_ASSERT would however still be the
default for the time being, unless let's say OSL_DIAGNOSE_FATAL is set. That
would allow for transition where just a part of code would
get -DOSL_DIAGNOSE_FATAL=1 in compiler flags and code could be slowly
converted, similarly like some people now do with -Werror. It would be a bit
harder to catch errors given that it's runtime (so I don't know if it would
be suitable for EasyHacks), but there would be $SAL_DIAGNOSE_DONT_ABORT for
such cases if one would run into something and wouldn't have time to fix it.
It probably would be even non-intrusive enough to be doable directly in
master, without a branch.
Does that sound ok?
> IIRC OOo had some plans
> to make at least smoketest completely 'assertion'-free.
Given the recent news from Oracle, it's a question if we can expect anything
from there. On the other side, if that's the case, it at least avoids the
problem of OOo introducing new code using OSL_ASSERT as non-fatal.
l.lunak at suse.cz
-------------- next part --------------
A non-text attachment was scrubbed...
Size: 565 bytes
Desc: not available
More information about the LibreOffice