Hi Bjorn,
very nice feature. Please see my comments marked with "magnus: " below.
On 11/08/2010 03:15 PM, Bjorn Munch wrote:
> #At file:///home/bm136801/my/ifexpr-55/ based on
> revid:bjorn.munch@stripped
>
> 3110 Bjorn Munch 2010-11-08
> Bug #57276 mysqltest: add support for simple compares in if/while conditions
> Added more parsing in do_block()
> Limitations: left operand must be variable, white space must surround
> operator
> Also changed var_set_int from 57036 to var_check_int
> Added tests to mysqltest.test
> Some tests can now be simplified but will take this later
> (This is a recommit of the first patch)
>
> modified:
> client/mysqltest.cc
> mysql-test/r/mysqltest.result
> mysql-test/t/mysqltest.test
> === modified file 'client/mysqltest.cc'
> --- a/client/mysqltest.cc 2010-10-21 09:20:53 +0000
> +++ b/client/mysqltest.cc 2010-11-08 14:13:45 +0000
> @@ -482,7 +482,8 @@ VAR* var_init(VAR* v, const char *name,
> int val_len);
> VAR* var_get(const char *var_name, const char** var_name_end,
> my_bool raw, my_bool ignore_not_existing);
> -void eval_expr(VAR* v, const char *p, const char** p_end);
> +void eval_expr(VAR* v, const char *p, const char** p_end,
> + my_bool open_end=FALSE);
magnus: use bool
magnus: the MySQL coding standard was recently update so you could also
use true/false now. But, just FYI. Leaving it up to you since
surrounding is still at TRUE/FALSE.
> my_bool match_delimiter(int c, const char *delim, uint length);
> void dump_result_to_reject_file(char *buf, int size);
> void dump_warning_messages();
> @@ -2040,9 +2041,11 @@ static void var_free(void *v)
>
> C_MODE_END
>
> -void var_set_int(VAR *v, const char *str)
> +void var_check_int(VAR *v)
> {
> char *endptr;
> + char *str= v->str_val;
magnus: maybe "assert(str)" ?
> +
> /* Initially assume not a number */
> v->int_val= 0;
> v->is_int= false;
> @@ -2089,7 +2092,7 @@ VAR *var_init(VAR *v, const char *name,
> memcpy(tmp_var->str_val, val, val_len);
> tmp_var->str_val[val_len]= 0;
> }
> - var_set_int(tmp_var, val);
> + var_check_int(tmp_var);
> tmp_var->name_len = name_len;
> tmp_var->str_val_len = val_len;
> tmp_var->alloced_len = val_alloc_len;
> @@ -2540,7 +2543,7 @@ void var_copy(VAR *dest, VAR *src)
> }
>
>
> -void eval_expr(VAR *v, const char *p, const char **p_end)
> +void eval_expr(VAR *v, const char *p, const char **p_end, my_bool open_end)
> {
>
> DBUG_ENTER("eval_expr");
> @@ -2558,7 +2561,7 @@ void eval_expr(VAR *v, const char *p, co
>
> /* Make sure there was just a $variable and nothing else */
> const char* end= *p_end + 1;
> - if (end< expected_end)
> + if (end< expected_end&& !open_end)
> die("Found junk '%.*s' after $variable in expression",
> (int)(expected_end - end - 1), end);
>
> @@ -2605,7 +2608,7 @@ void eval_expr(VAR *v, const char *p, co
> v->str_val_len = new_val_len;
> memcpy(v->str_val, p, new_val_len);
> v->str_val[new_val_len] = 0;
> - var_set_int(v, p);
> + var_check_int(v);
> }
> DBUG_VOID_RETURN;
> }
> @@ -5498,6 +5501,40 @@ int do_done(struct st_command *command)
> return 0;
> }
>
> +/* Operands available in if or while conditions */
> +
> +enum block_op {
> + EQ_OP,
> + NE_OP,
> + GT_OP,
> + GE_OP,
> + LT_OP,
> + LE_OP,
> + ILLEG_OP
> +};
> +
> +
> +enum block_op find_operand(const char *start)
> +{
> + char first= *start;
> + char next= *(start+1);
> +
> + if (first == '='&& next == '=')
> + return EQ_OP;
> + if (first == '!'&& next == '=')
> + return NE_OP;
> + if (first == '>'&& next == '=')
> + return GE_OP;
> + if (first == '>'&& next == ' ')
> + return GT_OP;
magnus: "next == ' '" implies that there must be at least one space
between operator and the following expr, is that really ok?
I bet someone will write
if ($variable>1)
and find it weird it does not work(I'm not saying that mysqltest
language isn't strange already :) )
You have written
if ($variable< 1) in test cases, which is not consistent.
> + if (first == '<'&& next == '=')
> + return LE_OP;
> + if (first == '<'&& next == ' ')
> + return LT_OP;
> +
> + return ILLEG_OP;
> +}
> +
>
> /*
> Process start of a "if" or "while" statement
> @@ -5523,6 +5560,13 @@ int do_done(struct st_command *command)
> A '!' can be used before the<expr> to indicate it should
> be executed if it evaluates to zero.
>
> +<expr> can also be a simple comparison condition:
> +
> +<variable> <op> <expr>
> +
> + The left hand side must be a variable, the right hand side can be a
> + variable, number, string or `query`. Operands are ==, !=,<,<=,>,>=.
> + == and != can be used for strings, all can be used for numerical values.
> */
>
> void do_block(enum block_cmd cmd, struct st_command* command)
> @@ -5558,6 +5602,9 @@ void do_block(enum block_cmd cmd, struct
> if (!expr_start++)
> die("missing '(' in %s", cmd_name);
>
> + while (my_isspace(charset_info, *expr_start))
> + expr_start++;
> +
> /* Check for !<expr> */
> if (*expr_start == '!')
> {
> @@ -5576,14 +5623,84 @@ void do_block(enum block_cmd cmd, struct
> die("Missing '{' after %s. Found \"%s\"", cmd_name, p);
>
> var_init(&v,0,0,0,0);
> - eval_expr(&v, expr_start,&expr_end);
>
> + /* If expression starts with a variable, it may be a compare condition */
> +
> + if (*expr_start == '$')
> + {
> + const char *curr_ptr= expr_end;
> + eval_expr(&v, expr_start,&curr_ptr, TRUE);
> + while (my_isspace(charset_info, *++curr_ptr)) ;
magnus: put "curr_ptr++" on next line for better readability, similar to
expr_start++ above.
> + /* If there was nothing past the variable, skip condition part */
> + if (curr_ptr == expr_end)
> + goto NO_COMPARE;
> +
> + enum block_op operand= find_operand(curr_ptr);
> + if (operand == ILLEG_OP)
> + die("Found junk '%.*s' after $variable in condition",
> + (int)(expr_end - curr_ptr), curr_ptr);
> +
> + /* We could silently allow this, but may be confusing */
> + if (not_expr)
> + die("Negation and comparison should not be combined, please rewrite");
> +
> + curr_ptr++;
magnus: What are you skipping, add comment please.
> + while (my_isspace(charset_info, *++curr_ptr)) ;
magnus: put curr_ptr on next line.
> +
> + VAR v2;
> + var_init(&v2,0,0,0,0);
> + eval_expr(&v2, curr_ptr,&expr_end);
> +
> + if ((operand!=EQ_OP&& operand!=NE_OP)&& ! (v.is_int&&
> v2.is_int))
> + die ("Cannot use inequality operand on non-numeric values");
magnus: Maybe the explanation should be reversed to tell what is
supported instead of telling what not is supported? :)
die("can only use == and != for string comparison");
> +
> + /* Now we overwrite the first variable with the value of 0 or 1 */
magnus: ..overwrite with the value of the expression ?
> +
> + switch (operand)
> + {
> + case EQ_OP:
> + if (v.is_int)
> + v.int_val= (v2.is_int&& v2.int_val == v.int_val);
magnus why is v2.is_int part of result? shouldn't it be just an
"assert(v2.is_int)" . Else give an example.
> + else
> + v.int_val= !strcmp (v.str_val, v2.str_val);
> + break;
> +
> + case NE_OP:
> + if (v.is_int)
> + v.int_val= ! (v2.is_int&& v2.int_val == v.int_val);
> + else
> + v.int_val= !! strcmp (v.str_val, v2.str_val);
magnus: hehe, or skip both negations ?
> + break;
> +
> + case LT_OP:
> + v.int_val= (v.int_val< v2.int_val);
> + break;
> + case LE_OP:
> + v.int_val= (v.int_val<= v2.int_val);
> + break;
> + case GT_OP:
> + v.int_val= (v.int_val> v2.int_val);
> + break;
> + case GE_OP:
> + v.int_val= (v.int_val>= v2.int_val);
> + break;
> + }
> +
> + v.is_int= TRUE;
> + } else
> + {
> + if (*expr_start != '`'&& ! my_isdigit(charset_info, *expr_start))
> + die("Expression in if/while must beging with $, ` or a number");
magnus: good syntax check.
> + eval_expr(&v, expr_start,&expr_end);
> + }
> +
> + NO_COMPARE:
> /* Define inner block */
> cur_block++;
> cur_block->cmd= cmd;
> - if (v.int_val)
> + if (v.is_int)
> {
> - cur_block->ok= TRUE;
> + cur_block->ok= (v.int_val != 0);
> } else
> /* Any non-empty string which does not begin with 0 is also TRUE */
> {
>
> === modified file 'mysql-test/r/mysqltest.result'
> --- a/mysql-test/r/mysqltest.result 2010-09-28 14:00:11 +0000
> +++ b/mysql-test/r/mysqltest.result 2010-11-08 14:13:45 +0000
> @@ -401,8 +401,31 @@ true-outer
> Counter is greater than 0, (counter=10)
> Counter is not 0, (counter=0)
> Counter is true, (counter=alpha)
> -Beta is true
> while with string, only once
> +5<7
> +5<6
> +5>=5
> +5>3
> +5==5
> +5!=8
> +5==3+2
> +5 == 5
> +hello == hello
> +hello == hello
> +hello != goodbye
> +two words
> +two words are two words
> +right answer
> +anything goes
> +mysqltest: At line 2: Cannot use inequality operand on non-numeric values
> +mysqltest: At line 2: Found junk '<> 6' after $variable in condition
> +mysqltest: At line 2: Expression in if/while must beging with $, ` or a number
> +counter is 2
> +counter is 3
> +counter is 4
> +counter is 5
> +counter is 6
> +counter is 7
> 1
> Testing while with not
> mysqltest: In included file "MYSQLTEST_VARDIR/tmp/mysqltest_while.inc": At line 64:
> Nesting too deeply
> @@ -807,8 +830,6 @@ dir-list.txt
> SELECT 'c:\\a.txt' AS col;
> col
> z
> -hej
> -mysqltest: At line 1: Found junk ' != 143' after $variable in expression
> select 1;
> 1
> 1
>
> === modified file 'mysql-test/t/mysqltest.test'
> --- a/mysql-test/t/mysqltest.test 2010-10-19 12:13:05 +0000
> +++ b/mysql-test/t/mysqltest.test 2010-11-08 14:13:45 +0000
> @@ -1163,10 +1163,11 @@ if ($counter)
> {
> echo oops, -0 is true;
> }
> -if (beta)
> -{
> - echo Beta is true;
> -}
> +# This is no longer allowed, as a precaution against mistyped conditionals
> +# if (beta)
> +# {
> +# echo Beta is true;
> +# }
magnus: good choice!
> let $counter=gamma;
> while ($counter)
> {
> @@ -1175,6 +1176,152 @@ while ($counter)
> }
>
> # ----------------------------------------------------------------------------
> +# Test if with compare conditions
> +# ----------------------------------------------------------------------------
> +
> +let $ifvar= 5;
> +let $ifvar2= 6;
> +
> +if ($ifvar< 7)
magnus: ^ so no space required before cond?
> +{
> + echo 5<7;
> +}
> +if ($ifvar< $ifvar2)
> +{
> + echo 5<6;
> +}
> +if ($ifvar<= 4)
> +{
> + echo 5<=4;
> +}
> +if ($ifvar>= 5)
> +{
> + echo 5>=5;
> +}
> +if ($ifvar> 3)
> +{
> + echo 5>3;
> +}
> +if ($ifvar == 4)
> +{
> + echo 5==4;
> +}
> +if ($ifvar == 5)
> +{
> + echo 5==5;
> +}
> +if ($ifvar != 8)
> +{
> + echo 5!=8;
> +}
> +if ($ifvar == `SELECT 3+2`)
> +{
> + echo 5==3+2;
magnus: cool feature!
> +}
> +if ($ifvar == 5)
> +{
> + echo 5 == 5;
> +}
> +let $ifvar= hello;
> +if ($ifvar == hello there)
> +{
> + echo hello == hello there;
> +}
> +if ($ifvar == hello)
> +{
> + echo hello == hello;
> +}
> +if ($ifvar == hell)
> +{
> + echo hello == hell;
> +}
> +if ($ifvar == hello)
> +{
> + echo hello == hello;
> +}
> +if ($ifvar != goodbye)
> +{
> + echo hello != goodbye;
> +}
> +
> +let $ifvar= two words;
> +if ($ifvar == two words)
> +{
> + echo two words;
> +}
> +if ($ifvar == `SELECT 'two words'`)
> +{
> + echo two words are two words;
> +}
> +if (42)
> +{
> + echo right answer;
> +}
> +if (0)
> +{
> + echo wrong answer;
> +}
> +if (`SELECT 'something'`)
> +{
> + echo anything goes;
> +}
> +
magnus: Add comment why test checks
> +--write_file $MYSQL_TMP_DIR/mysqltest.sql
> +let $var= 5;
> +if ($var>= four)
> +{
> + echo 5>=four;
> +}
> +EOF
> +--error 1
> +--exec $MYSQL_TEST< $MYSQL_TMP_DIR/mysqltest.sql 2>&1
> +remove_file $MYSQL_TMP_DIR/mysqltest.sql;
> +
> +--write_file $MYSQL_TMP_DIR/mysqltest.sql
> +let $var= 5;
> +if ($var<> 6)
> +{
> + echo 5<>6;
> +}
> +EOF
> +--error 1
> +--exec $MYSQL_TEST< $MYSQL_TMP_DIR/mysqltest.sql 2>&1
> +remove_file $MYSQL_TMP_DIR/mysqltest.sql;
> +
> +--write_file $MYSQL_TMP_DIR/mysqltest.sql
> +let $var= text;
> +if (var == text)
> +{
> + echo Oops I forgot the $;
> +}
> +EOF
> +--error 1
> +--exec $MYSQL_TEST< $MYSQL_TMP_DIR/mysqltest.sql 2>&1
> +remove_file $MYSQL_TMP_DIR/mysqltest.sql;
> +
> +# ----------------------------------------------------------------------------
> +# Test while with compare conditions
> +# ----------------------------------------------------------------------------
> +
> +let $counter= 2;
> +
> +while ($counter< 5)
> +{
> + echo counter is $counter;
> + inc $counter;
> +}
> +let $ifvar=;
> +while ($ifvar != stop)
> +{
> + if ($counter>= 7)
> + {
> + let $ifvar= stop;
> + }
> + echo counter is $counter;
> + inc $counter;
> +}
> +
> +# ----------------------------------------------------------------------------
> # Test while, { and }
> # ----------------------------------------------------------------------------
>
> @@ -2529,26 +2676,6 @@ rmdir $MYSQLTEST_VARDIR/tmp/testdir;
> --replace_result c:\\a.txt z
> SELECT 'c:\\a.txt' AS col;
>
> -#
> -# Bug#32307 mysqltest - does not detect illegal if syntax
> -#
> -
> -let $test= 1;
> -if ($test){
> - echo hej;
> -}
> -
> ---write_file $MYSQLTEST_VARDIR/tmp/mysqltest.sql
> -if ($mysql_errno != 1436)
> -{
> - echo ^ Should not be allowed!
> -}
> -EOF
> ---error 1
> ---exec $MYSQL_TEST< $MYSQLTEST_VARDIR/tmp/mysqltest.sql 2>&1
> -remove_file $MYSQLTEST_VARDIR/tmp/mysqltest.sql;
> -
> -
> # ----------------------------------------------------------------------------
> # Test that -- is not allowed as comment, only as mysqltest builtin command
> # ----------------------------------------------------------------------------
>
>
>
>
>
