List:Commits« Previous MessageNext Message »
From:Magnus Blåudd Date:November 8 2010 2:54pm
Subject:Re: bzr commit into mysql-5.5-mtr branch (bjorn.munch:3110) Bug#57276
View as plain text  
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
>   # ----------------------------------------------------------------------------
>
>
>
>
>

Thread
bzr commit into mysql-5.5-mtr branch (bjorn.munch:3110) Bug#57276Bjorn Munch8 Nov
  • Re: bzr commit into mysql-5.5-mtr branch (bjorn.munch:3110) Bug#57276Magnus Blåudd8 Nov
    • Re: bzr commit into mysql-5.5-mtr branch (bjorn.munch:3110)Bug#57276Bjorn Munch9 Nov
      • Re: bzr commit into mysql-5.5-mtr branch (bjorn.munch:3110) Bug#57276Magnus Blåudd9 Nov