tdf124710, unexpected result for function IFS when argument NA() is followed by an argument that needs interpreting

[Eike Rathke]

Hi Winfried,

On Wednesday, 2019-04-17 11:54:13 +0000, Winfried Donkers wrote:

> Upon debugging IFS( 0, NA(), 1+0, "output" ) I came to the following preliminary conclusions:
> Before ScInterpreter::ScIfs_MS() is called, ScInterpreter::Interpret is called for argument '1+0' .
> That leads to a call to scInterpreter::CalculateAddSub( false ) and there the problem starts.
> 0 and 1 are popped and added, but after PushDouble( ::rtl::math::approxAdd( 0, 1 ) )
> the raw stacktype is svError.

That happens because nGlobalError is still set from the previous NA()
call, any Push...() pushes the error value instead of a result if
nGlobalError is set. Which is desired to propagate the error, and
normally doesn't happen this way (carrying over from the previous
argument) unless an OpCode is part of IsErrFunc() (which ocIfs_MS and
ocSwitch_MS are, likely because of similar reasons, but it's only
a partial workaround), otherwise calculation ends with the error.

The actual root cause here is that IFS() and SWITCH() are not compiled
into jumps so only the needed result would be calculated, but instead
all arguments are calculated beforehand as in usual function calls. We
had that discussion some while ago (and there exists some bug about it),
but I didn't get around to a different implementation. (right now I only
remember I had some idea for it to use existing means to compile an
IFS() call into some kind of nested or chained ocIf or ocChoose or some
such, would have to dig it out of my mail archive).

A workaround might be that for pre-calculated results of IFS() and
SWITCH() nGlobalError is cleared before stepping on to the next
argument, but that could get hairy determining the right spot when to do
it, i.e. when the same level of calls is reached. Maybe the place with
the IsErrFunc() handling in ScInterpreter::Interpret() can be used for
that, though determining whether the current OpCode is in the argument
level of IFS() or SWITCH() probably isn't too trivial.

Not treating IFS() and SWITCH() as jumps of course code paths are
unnecessarily executed and has other unwanted side effects (like there
were cases where someone used STYLE() within but not the actual final
result), so the best fix would actually be to change that.

  Eike

-- 
GPG key 0x6A6CD5B765632D3A - 2265 D7F3 A7B0 95CC 3918  630B 6A6C D5B7 6563 2D3A
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/libreoffice/attachments/20190417/c70d89eb/attachment.sig>