PATCH: math error messages

PATCH: math error messages

[Peter Stephenson]

On Mon, 28 Oct 2013 17:55:05 +0000
Peter Stephenson <p.stephenson@samsung.com> wrote:
> >     zsh: bad math expression: operand expected at `'
>
> The error you got comes from it being empty, so there's no
> number, which is what it's complaining about, slightly cryptically.

Is anyone particularly attached to the current error messages?  I think
the following are a bit clearer for the two specific cases, although I'm
not sure about the word "string".  I used it in order to keep the
standard math error prefix and avoid repeating "expression".  I could
say "argument" but that means a lot of possible things, most commonly a
command line argument which would be inaccurate.

% print $foo[]
zsh: bad math expression: empty string
% print $foo[3+]
zsh: bad math expression: operand expected at end of string

I have not changed any behaviour, only messages.  I've noted that while
$(()) returns 0, $foo[$unset] is an error.  I'm happy with the latter
and I think the former was forced on us by standards.

diff --git a/Src/math.c b/Src/math.c
index eae283d..b21a3ad 100644
--- a/Src/math.c
+++ b/Src/math.c
@@ -1374,6 +1374,19 @@ mathevalarg(char *s, char **ss)
     mnumber x;
     int xmtok = mtok;
 
+    /*
+     * At this entry point we don't allow an empty expression,
+     * whereas we do with matheval().  I'm not sure if this
+     * difference is deliberate, but it does mean that e.g.
+     * $array[$ind] where ind hasn't been set produces an error,
+     * which is probably safe.
+     *
+     * To avoid a more opaque error further in, bail out here.
+     */
+    if (!*s) {
+	zerr("bad math expression: empty string");
+	return (zlong)0;
+    }
     x = mathevall(s, MPREC_ARG, ss);
     if (mtok == COMMA)
 	(*ss)--;
@@ -1401,6 +1414,7 @@ checkunary(int mtokc, char *mptr)
     }
     if (errmsg) {
 	int len, over = 0;
+	char *errtype = errmsg == 2 ? "operator" : "operand";
 	while (inblank(*mptr))
 	    mptr++;
 	len = ztrlen(mptr);
@@ -1408,9 +1422,12 @@ checkunary(int mtokc, char *mptr)
 	    len = 10;
 	    over = 1;
 	}
-	zerr("bad math expression: %s expected at `%l%s'",
-	     errmsg == 2 ? "operator" : "operand",
-	     mptr, len, over ? "..." : "");
+	if (!*mptr)
+	    zerr("bad math expression: %s expected at end of string",
+		errtype);
+	else
+	    zerr("bad math expression: %s expected at `%l%s'",
+		 errtype, mptr, len, over ? "..." : "");
     }
     unary = !(tp & OP_OPF);
 }
diff --git a/Test/C01arith.ztst b/Test/C01arith.ztst
index 71c8a19..c19135c 100644
--- a/Test/C01arith.ztst
+++ b/Test/C01arith.ztst
@@ -134,11 +134,11 @@
 
   print $(( a = ))
 1:empty assignment
-?(eval):1: bad math expression: operand expected at `'
+?(eval):1: bad math expression: operand expected at end of string
 
   print $(( 3, ))
 1:empty right hand of comma
-?(eval):1: bad math expression: operand expected at `'
+?(eval):1: bad math expression: operand expected at end of string
 
   print $(( 3,,4 ))
 1:empty middle of comma

pws

Re: PATCH: math error messages

[Greg Klanderman]

I don't think most users would realize that "bad math expression"
might be referring to an array indexing expression.  Seems like
ideally, if you can tell you're in an array index context, it would
say "empty array index".  "math expression" is good for $((...)).
not sure what other contexts we could be in here.

Maybe even better, have the error prefix reflect the context: "bad
array index: empty string".

Not sure you even need "string", seems you could just drop it in
the two cases below.

Greg

>>>>> On October 29, 2013 Peter Stephenson <p.stephenson@samsung.com> wrote:

> On Mon, 28 Oct 2013 17:55:05 +0000
> Peter Stephenson <p.stephenson@samsung.com> wrote:
>> >     zsh: bad math expression: operand expected at `'
>> 
>> The error you got comes from it being empty, so there's no
>> number, which is what it's complaining about, slightly cryptically.

> Is anyone particularly attached to the current error messages?  I think
> the following are a bit clearer for the two specific cases, although I'm
> not sure about the word "string".  I used it in order to keep the
> standard math error prefix and avoid repeating "expression".  I could
> say "argument" but that means a lot of possible things, most commonly a
> command line argument which would be inaccurate.

> % print $foo[]
> zsh: bad math expression: empty string
> % print $foo[3+]
> zsh: bad math expression: operand expected at end of string

> I have not changed any behaviour, only messages.  I've noted that while
> $(()) returns 0, $foo[$unset] is an error.  I'm happy with the latter
> and I think the former was forced on us by standards.

> diff --git a/Src/math.c b/Src/math.c
> index eae283d..b21a3ad 100644
> --- a/Src/math.c
> +++ b/Src/math.c
> @@ -1374,6 +1374,19 @@ mathevalarg(char *s, char **ss)
>      mnumber x;
>      int xmtok = mtok;

> +    /*
> +     * At this entry point we don't allow an empty expression,
> +     * whereas we do with matheval().  I'm not sure if this
> +     * difference is deliberate, but it does mean that e.g.
> +     * $array[$ind] where ind hasn't been set produces an error,
> +     * which is probably safe.
> +     *
> +     * To avoid a more opaque error further in, bail out here.
> +     */
> +    if (!*s) {
> +	zerr("bad math expression: empty string");
> +	return (zlong)0;
> +    }
>      x = mathevall(s, MPREC_ARG, ss);
>      if (mtok == COMMA)
>  	(*ss)--;
> @@ -1401,6 +1414,7 @@ checkunary(int mtokc, char *mptr)
>      }
>      if (errmsg) {
>  	int len, over = 0;
> +	char *errtype = errmsg == 2 ? "operator" : "operand";
>  	while (inblank(*mptr))
>  	    mptr++;
>  	len = ztrlen(mptr);
> @@ -1408,9 +1422,12 @@ checkunary(int mtokc, char *mptr)
>  	    len = 10;
>  	    over = 1;
>  	}
> -	zerr("bad math expression: %s expected at `%l%s'",
> -	     errmsg == 2 ? "operator" : "operand",
> -	     mptr, len, over ? "..." : "");
> +	if (!*mptr)
> +	    zerr("bad math expression: %s expected at end of string",
> +		errtype);
> +	else
> +	    zerr("bad math expression: %s expected at `%l%s'",
> +		 errtype, mptr, len, over ? "..." : "");
>      }
>      unary = !(tp & OP_OPF);
>  }
> diff --git a/Test/C01arith.ztst b/Test/C01arith.ztst
> index 71c8a19..c19135c 100644
> --- a/Test/C01arith.ztst
> +++ b/Test/C01arith.ztst
> @@ -134,11 +134,11 @@

>    print $(( a = ))
>  1:empty assignment
> -?(eval):1: bad math expression: operand expected at `'
> +?(eval):1: bad math expression: operand expected at end of string

>    print $(( 3, ))
>  1:empty right hand of comma
> -?(eval):1: bad math expression: operand expected at `'
> +?(eval):1: bad math expression: operand expected at end of string

>    print $(( 3,,4 ))
>  1:empty middle of comma

> pws