expr: Improve mathfunc handling

Previously math functions with multiple arguments were handling poorly.

For example:      pow(1.0 + 3.0 - 2, .8 * 5)
was evaluated as: pow(1.0 + 3.0, 2 - .8 * 5)

Now commas correctly separate function parameters.

Also, the error messages are more consistent and informative in the case
of an invalid expression.

Update and re-enable tests in tests/expr-old.test accordingly

Signed-off-by: Steve Bennett <steveb@workware.net.au>

2 files changed, 113 insertions, 76 deletions

diff --git a/jim.c b/jim.c
index ca3bcec..9e41d8c 100644
--- a/jim.c
+++ b/jim.c
@@ -8646,6 +8646,12 @@ const char *jim_tt_name(int type)
     if (type < JIM_TT_EXPR_OP) {
         return tt_names[type];
     }
+    else if (type == JIM_EXPROP_UNARYMINUS) {
+        return "-VE";
+    }
+    else if (type == JIM_EXPROP_UNARYPLUS) {
+        return "+VE";
+    }
     else {
         const struct Jim_ExprOperator *op = JimExprOperatorInfoByOpcode(type);
         static char buf[20];
@@ -8714,12 +8720,16 @@ static void DupExprInternalRep(Jim_Interp *interp, Jim_Obj *srcPtr, Jim_Obj *dup
     dupPtr->typePtr = NULL;
 }
 
-/* Check if an expr program looks correct. */
-static int ExprCheckCorrectness(ExprByteCode * expr)
+/* Check if an expr program looks correct
+ * Sets an error result on invalid
+ */
+static int ExprCheckCorrectness(Jim_Interp *interp, Jim_Obj *exprObjPtr, ExprByteCode * expr)
 {
     int i;
     int stacklen = 0;
     int ternary = 0;
+    int lasttt = JIM_TT_NONE;
+    const char *errmsg;
 
     /* Try to check if there are stack underflows,
      * and make sure at the end of the program there is
@@ -8727,8 +8737,10 @@ static int ExprCheckCorrectness(ExprByteCode * expr)
     for (i = 0; i < expr->len; i++) {
         ScriptToken *t = &expr->token[i];
         const struct Jim_ExprOperator *op = JimExprOperatorInfoByOpcode(t->type);
+        lasttt = t->type;
 
         stacklen -= op->arity;
+
         if (stacklen < 0) {
             break;
         }
@@ -8742,10 +8754,31 @@ static int ExprCheckCorrectness(ExprByteCode * expr)
         /* All operations and operands add one to the stack */
         stacklen++;
     }
-    if (stacklen != 1 || ternary != 0) {
-        return JIM_ERR;
+    if (stacklen == 1 && ternary == 0) {
+        return JIM_OK;
     }
-    return JIM_OK;
+
+    if (stacklen <= 0) {
+        /* Too few args */
+        if (lasttt >= JIM_EXPROP_FUNC_FIRST) {
+            errmsg = "too few arguments for math function";
+            Jim_SetResultString(interp, "too few arguments for math function", -1);
+        } else {
+            errmsg = "premature end of expression";
+        }
+    }
+    else if (stacklen > 1) {
+        if (lasttt >= JIM_EXPROP_FUNC_FIRST) {
+            errmsg = "too many arguments for math function";
+        } else {
+            errmsg = "extra tokens at end of expression";
+        }
+    }
+    else {
+        errmsg = "invalid ternary expression";
+    }
+    Jim_SetResultFormatted(interp, "syntax error in expression \"%#s\": %s", exprObjPtr, errmsg);
+    return JIM_ERR;
 }
 
 /* This procedure converts every occurrence of || and && opereators
@@ -9092,19 +9125,19 @@ strexpr:
 
             case JIM_TT_SUBEXPR_START:
                 Jim_StackPush(&stack, t);
-                prevtt = JIM_TT_NONE;
-                continue;
-
-            case JIM_TT_SUBEXPR_COMMA:
-                /* Simple approach. Comma is simply ignored */
-                continue;
+                break;
 
             case JIM_TT_SUBEXPR_END:
+            case JIM_TT_SUBEXPR_COMMA:
                 ok = 0;
                 while (Jim_StackLen(&stack)) {
                     ParseToken *tt = Jim_StackPop(&stack);
 
-                    if (tt->type == JIM_TT_SUBEXPR_START) {
+                    if (tt->type == JIM_TT_SUBEXPR_START || tt->type == JIM_TT_SUBEXPR_COMMA) {
+                        if (t->type == JIM_TT_SUBEXPR_COMMA) {
+                            /* Need to push back the previous START or COMMA in the case of comma */
+                            Jim_StackPush(&stack, tt);
+                        }
                         ok = 1;
                         break;
                     }
@@ -9119,14 +9152,13 @@ strexpr:
                 }
                 break;
 
-
             default:{
                     /* Must be an operator */
                     const struct Jim_ExprOperator *op;
                     ParseToken *tt;
 
                     /* Convert -/+ to unary minus or unary plus if necessary */
-                    if (prevtt == JIM_TT_NONE || prevtt >= JIM_TT_EXPR_OP) {
+                    if (prevtt == JIM_TT_NONE || prevtt == JIM_TT_SUBEXPR_START || prevtt == JIM_TT_SUBEXPR_COMMA || prevtt >= JIM_TT_EXPR_OP) {
                         if (t->type == JIM_EXPROP_SUB) {
                             t->type = JIM_EXPROP_UNARYMINUS;
                         }
@@ -9231,7 +9263,6 @@ static int SetExprFromAny(Jim_Interp *interp, struct Jim_Obj *objPtr)
     while (!parser.eof) {
         if (JimParseExpression(&parser) != JIM_OK) {
             ScriptTokenListFree(&tokenlist);
-          invalidexpr:
             Jim_SetResultFormatted(interp, "syntax error in expression: \"%#s\"", objPtr);
             expr = NULL;
             goto err;
@@ -9282,9 +9313,11 @@ static int SetExprFromAny(Jim_Interp *interp, struct Jim_Obj *objPtr)
 #endif
 
     /* Check program correctness. */
-    if (ExprCheckCorrectness(expr) != JIM_OK) {
+    if (ExprCheckCorrectness(interp, objPtr, expr) != JIM_OK) {
+        /* ExprCheckCorrectness set an error in this case */
         ExprFreeByteCode(interp, expr);
-        goto invalidexpr;
+        expr = NULL;
+        goto err;
     }
 
     rc = JIM_OK;
diff --git a/tests/expr-old.test b/tests/expr-old.test
index 1021041..0bd9b53 100644
--- a/tests/expr-old.test
+++ b/tests/expr-old.test
@@ -384,7 +384,7 @@ test expr-old-23.8 {double quotes} {
 # Numbers in various bases.
 
 test expr-old-24.1 {numbers in different bases} {expr 0x20} 32
-test expr-old-24.2 {numbers in different bases} {expr 015} 15
+test expr-old-24.2 {numbers in different bases} {expr 0o015} 13
 
 # Conversions between various data types.
 
@@ -689,76 +689,80 @@ test expr-old-32.38 {math functions in expressions} {
 #test expr-old-32.40 {math functions in expressions} {
 #    list [catch {expr round(-1e60)} msg] $msg
 #} {1 {integer value too large to represent}}
-if {0} {
-test expr-old-32.41 {math functions in expressions} {
+test expr-old-32.41 {math functions in expressions} mathfunc {
     list [catch {expr pow(1.0 + 3.0 - 2, .8 * 5)} msg] $msg
 } {0 16.0}
-test expr-old-32.42 {math functions in expressions} {
+if {1} {
+test expr-old-32.42 {math functions in expressions} expr_hypot {
     list [catch {expr hypot(5*.8,3)} msg] $msg
 } {0 5.0}
+test expr-old-32.43 {math functions in expressions} mathfunc {
+    expr {pow(1.0 + 3.0, -2)}
+} {0.0625}
 test expr-old-32.45 {math functions in expressions} {
     expr (0 <= rand()) && (rand() < 1)
 } {1}
-test expr-old-32.46 {math functions in expressions} {
-    list [catch {expr rand(24)} msg] $msg
-} {1 {too many arguments for math function}}
-test expr-old-32.47 {math functions in expressions} {
-    list [catch {expr srand()} msg] $msg
+test expr-old-32.46 {math functions in expressions} -body {
+    expr rand(24)
+} -returnCodes error -match glob -result {*too many arguments for math function*}
+test expr-old-32.47 {math functions in expressions} -body {
+    expr srand()
+} -returnCodes error -match glob -result {*too few arguments for math function*}
 } {1 {too few arguments for math function}}
-test expr-old-32.48 {math functions in expressions} {
-    list [catch {expr srand(3.79)} msg] $msg
-} {1 {can't use floating-point value as argument to srand}}
-test expr-old-32.49 {math functions in expressions} {
-    list [catch {expr srand("")} msg] $msg
-} {1 {argument to math function didn't have numeric value}}
-test expr-old-32.50 {math functions in expressions} {
-    set result [expr round(srand(12345) * 1000)]
+test expr-old-32.48 {math functions in expressions} -body {
+    expr srand(3.79)
+} -returnCodes error -match glob -result *
+test expr-old-32.49 {math functions in expressions} -body {
+    expr srand("")
+} -returnCodes error -match glob -result *
+test expr-old-32.50 {math functions in expressions} mathfunc {
     for {set i 0} {$i < 10} {incr i} {
-	lappend result [expr round(rand() * 1000)]
+	lappend result [expr round(sin($i) * 1000)]
     }
     set result
-} {97 834 948 36 12 51 766 585 914 784 333}
-test expr-old-32.51 {math functions in expressions} {
-    list [catch {expr {srand([lindex "6ty" 0])}} msg] $msg
-} {1 {argument to math function didn't have numeric value}}
+} {0 841 909 141 -757 -959 -279 657 989 412}
+test expr-old-32.51 {math functions in expressions} -body {
+    expr {srand([lindex "6ty" 0])}
+} -returnCodes error -match glob -result *
 
-test expr-old-33.1 {conversions and fancy args to math functions} {
+test expr-old-33.1 {conversions and fancy args to math functions} expr_hypot {
     expr hypot ( 3 , 4 )
 } 5.0
-test expr-old-33.2 {conversions and fancy args to math functions} {
+test expr-old-33.2 {conversions and fancy args to math functions} expr_hypot {
     expr hypot ( (2.0+1.0) , 4 )
 } 5.0
-test expr-old-33.3 {conversions and fancy args to math functions} {
+test expr-old-33.3 {conversions and fancy args to math functions} expr_hypot {
     expr hypot ( 3 , (3.0 + 1.0) )
 } 5.0
-test expr-old-33.4 {conversions and fancy args to math functions} {
+test expr-old-33.4 {conversions and fancy args to math functions} mathfunc {
     format %.6g [expr cos(acos(0.1))]
 } 0.1
 
-test expr-old-34.1 {errors in math functions} {
-    list [catch {expr func_2(1.0)} msg] $msg
-} {1 {unknown math function "func_2"}}
-test expr-old-34.2 {errors in math functions} {
-    list [catch {expr func|(1.0)} msg] $msg
-} {1 {syntax error in expression "func|(1.0)"}}
-test expr-old-34.3 {errors in math functions} {
-    list [catch {expr {hypot("a b", 2.0)}} msg] $msg
-} {1 {argument to math function didn't have numeric value}}
-test expr-old-34.4 {errors in math functions} {
-    list [catch {expr hypot(1.0 2.0)} msg] $msg
-} {1 {syntax error in expression "hypot(1.0 2.0)"}}
-test expr-old-34.5 {errors in math functions} {
-    list [catch {expr hypot(1.0, 2.0} msg] $msg
-} {1 {syntax error in expression "hypot(1.0, 2.0"}}
-test expr-old-34.6 {errors in math functions} {
-    list [catch {expr hypot(1.0 ,} msg] $msg
-} {1 {syntax error in expression "hypot(1.0 ,"}}
-test expr-old-34.7 {errors in math functions} {
-    list [catch {expr hypot(1.0)} msg] $msg
-} {1 {too few arguments for math function}}
-test expr-old-34.8 {errors in math functions} {
-    list [catch {expr hypot(1.0, 2.0, 3.0)} msg] $msg
-} {1 {too many arguments for math function}}
+test expr-old-34.1 {errors in math functions} -body {
+    expr func_2(1.0)
+} -returnCodes error -match glob -result *
+test expr-old-34.2 {errors in math functions} -body {
+    expr {func|(1.0)}
+} -returnCodes error -match glob -result *
+test expr-old-34.3 {errors in math functions} -body {
+    expr {hypot("a b", 2.0)}
+} -returnCodes error -match glob -result *
+test expr-old-34.4 {errors in math functions} -constraints tcl -body {
+    expr {hypot(1.0 2.0)}
+} -returnCodes error -match glob -result *
+test expr-old-34.5 {errors in math functions} -body {
+    expr {hypot(1.0, 2.0}
+} -returnCodes error -match glob -result *
+test expr-old-34.6 {errors in math functions} -body {
+    expr {hypot(1.0 ,}
+} -returnCodes error -match glob -result *
+test expr-old-34.7 {errors in math functions} -constraints expr_hypot -body {
+    expr hypot(1.0)
+} -returnCodes error -match glob -result {*too few arguments for math function*}
+test expr-old-34.8 {errors in math functions} -constraints expr_hypot -body {
+    expr {hypot(1.0, 2.0, 3.0)}
+} -returnCodes error -match glob -result {*too many arguments for math function*}
+if 0 {
 test expr-old-34.9 {errors in math functions} {
     list [catch {expr acos(-2.0)} msg] $msg $errorCode
 } {1 {domain error: argument not in valid range} {ARITH DOMAIN {domain error: argument not in valid range}}}
@@ -783,11 +787,12 @@ test expr-old-34.15 {errors in math functions} {
 test expr-old-34.16 {errors in math functions} {
     list [catch {expr round(-1.0e30)} msg] $msg $errorCode
 } {1 {integer value too large to represent} {ARITH IOVERFLOW {integer value too large to represent}}}
+}
 
-test expr-old-36.1 {ExprLooksLikeInt procedure} {
-    list [catch {expr 0289} msg] $msg
-} {1 {"0289" is an invalid octal number}}
-test expr-old-36.2 {ExprLooksLikeInt procedure} {
+test expr-old-36.1 {ExprLooksLikeInt procedure} -constraints tcl -body {
+    expr 0289
+} -returnCodes error -match glob -result *
+test expr-old-36.2 {ExprLooksLikeInt procedure} tcl {
     set x 0289
     list [catch {expr {$x+1}} msg] $msg
 } {1 {can't use invalid octal number as operand of "+"}}
@@ -806,7 +811,7 @@ test expr-old-36.6 {ExprLooksLikeInt procedure} {
     set x {	-22}
     list [catch {expr {$x+1}} msg] $msg
 } {0 -21}
-test expr-old-36.7 {ExprLooksLikeInt procedure} {
+test expr-old-36.7 {ExprLooksLikeInt procedure} tcl {
     list [catch {expr nan} msg] $msg
 } {1 {domain error: argument not in valid range}}
 test expr-old-36.8 {ExprLooksLikeInt procedure} {
@@ -815,10 +820,9 @@ test expr-old-36.8 {ExprLooksLikeInt procedure} {
 test expr-old-36.9 {ExprLooksLikeInt procedure} {
     list [catch {expr 24E1} msg] $msg
 } {0 240.0}
-}
-test expr-old-36.10 {ExprLooksLikeInt procedure} {
-    list [catch {expr 78e} msg] $msg
-} {1 {syntax error in expression: "78e"}}
+test expr-old-36.10 {ExprLooksLikeInt procedure} -constraints tcl -body {
+    expr 78e
+} -returnCodes error -match glob -result *
 
 # test for [Bug #542588]
 # XXX: Can't rely on overflow checking