PATCH: error status from math evaluation

[Peter Stephenson <pws@xxxxxxx>]

Playing with parameters, I discovered that an error that would normally
be fatal (i.e., that sets the internal variable errflag) in (( ... ))
not only resets the error flag, it returns status zero.  For example

% readonly readonly
% (( readonly = 1 )) && print Success
zsh: read-only variable: readonly
Success

That combination surely can't be right.

Resetting the error flag (after detecting the error) is probably OK;
that just means the script or function etc. doesn't bomb out at that
point.  I wouldn't want to change that now since it has quite wide
implications for execution and error handling.

The status zero comes from the fact that we explicitly set the returned
integer to the value of errflag if that's non-zero, and then we use that
value to test for the status; the non-zero value produces zero return
status.  This is rather screwy; I can only imagine that someone thought
"well, the value is probably wrong so I can't return that... let's
return errflag instead."  But the value isn't useful and the caller
hasn't been given a way of detecting this has happened.

Suppose we return status 2 on an error?  That's consistent with other
bits of the shell and gives something to test for.  I don't see any
reason to maintain compatibility---the current code is just too obscure
to be useful.

I've set the returned value from mathevall() to zero instead of errflag,
which seems to make more sense.  At the moment I can't see a way the old
behaviour could ever be useful.  The value is irrelevant for (( ... ))
because of the previous fix, and it's irrelevant for $(( ... )) because
we don't reset the error flag in that case:

% print $(( readonly = 1 )); echo Got here
zsh: read-only variable: readonly

(no further output because the command sequence was aborted.)  This is
inconsistent but arguable---in this form you haven't got the chance to
handle a bad return status so an exception is all that's left.

In summary, the only visible change is status 2 instead of 0 on an error
from (( ... )).

Index: Doc/Zsh/builtins.yo
===================================================================
RCS file: /cvsroot/zsh/zsh/Doc/Zsh/builtins.yo,v
retrieving revision 1.94
diff -u -r1.94 builtins.yo
--- Doc/Zsh/builtins.yo	28 May 2007 22:57:40 -0000	1.94
+++ Doc/Zsh/builtins.yo	11 Jun 2007 15:24:36 -0000
@@ -708,7 +708,8 @@
 ifzman(the section `Arithmetic Evaluation' in zmanref(zshmisc))\
 ifnzman(noderef(Arithmetic Evaluation))
 for a description of arithmetic expressions.  The exit status is 0 if the
-value of the last expression is nonzero, and 1 otherwise.
+value of the last expression is nonzero, 1 if it is zero, and 2 if
+an error occurred.
 )
 findex(limit)
 cindex(resource limits)
Index: Doc/Zsh/arith.yo
===================================================================
RCS file: /cvsroot/zsh/zsh/Doc/Zsh/arith.yo,v
retrieving revision 1.10
diff -u -r1.10 arith.yo
--- Doc/Zsh/arith.yo	30 Jun 2006 09:41:35 -0000	1.10
+++ Doc/Zsh/arith.yo	11 Jun 2007 15:24:36 -0000
@@ -22,7 +22,7 @@
 arithmetic expansion performed as for an argument of tt(let).  More
 precisely, `tt(LPAR()LPAR())var(...)tt(RPAR()RPAR())' is equivalent to
 `tt(let ")var(...)tt(")'.  The return status is 0 if the arithmetic value
-of the expression is non-zero, and 1 otherwise.
+of the expression is non-zero, 1 if it is zero, and 2 if an error occurred.
 
 For example, the following statement
 
Index: Src/exec.c
===================================================================
RCS file: /cvsroot/zsh/zsh/Src/exec.c,v
retrieving revision 1.116
diff -u -r1.116 exec.c
--- Src/exec.c	3 Jun 2007 17:44:20 -0000	1.116
+++ Src/exec.c	11 Jun 2007 15:24:37 -0000
@@ -3662,7 +3662,10 @@
 	fprintf(xtrerr, " ))\n");
 	fflush(xtrerr);
     }
-    errflag = 0;
+    if (errflag) {
+	errflag = 0;
+	return 2;
+    }
     /* should test for fabs(val.u.d) < epsilon? */
     return (val.type == MN_INTEGER) ? val.u.l == 0 : val.u.d == 0.0;
 }
Index: Src/math.c
===================================================================
RCS file: /cvsroot/zsh/zsh/Src/math.c,v
retrieving revision 1.29
diff -u -r1.29 math.c
--- Src/math.c	12 Feb 2007 16:43:42 -0000	1.29
+++ Src/math.c	11 Jun 2007 15:24:37 -0000
@@ -1061,8 +1061,19 @@
 	  "BUG: math: wallabies roaming too freely in outback");
 
     if (errflag) {
+	/*
+	 * This used to set the return value to errflag.
+	 * I don't understand how that could be useful; the
+	 * caller doesn't know that's what's happened and
+	 * may not get a value at all.
+	 * Worse, we reset errflag in execarith() and setting
+	 * this explicitly non-zero means a (( ... )) returns
+	 * status 0 if there's an error.  That surely can't
+	 * be right.  execarith() now detects an error and returns
+	 * status 2.
+	 */
 	ret.type = MN_INTEGER;
-	ret.u.l = errflag;
+	ret.u.l = 0;
     } else {
 	if (stack[0].val.type == MN_UNSET)
 	    ret = getnparam(stack[0].lval);

-- 
Peter Stephenson <pws@xxxxxxx>                  Software Engineer
CSR PLC, Churchill House, Cambridge Business Park, Cowley Road
Cambridge, CB4 0WZ, UK                          Tel: +44 (0)1223 692070


To access the latest news from CSR copy this link into a web browser:  http://www.csr.com/email_sig.php

To get further information regarding CSR, please visit our Investor Relations page at http://ir.csr.com/csr/about/overview