From: Bjorn Munch
Date: November 9 2010 3:30pm
Subject: Re: bzr commit into mysql-5.5-mtr branch (bjorn.munch:3110)
 Bug#57276
List-Archive: http://lists.mysql.com/commits/123298
Message-Id: <20101109153005.GA14060@khepri15.norway.sun.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Transfer-Encoding: 8bit

On 08/11 15.54, Magnus Blåudd wrote:
> Hi Bjorn,
> 
> very nice feature. Please see my comments marked with "magnus: " below.

I have commented your comments.

> >-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.

OK.

> >-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)" ?

No, because str can actually be a null-ptr here, in which case we
return early.

> >+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?

OK I have fixed this. GT_OP and LT_OP will not check next.
 
> 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 :) )

I have fixed it so that white space is no longer needed.

> >+    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.

Not possible due to pre-increment, but I've replaced ; with a {} on a
separate line.
 
> >+    /* 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.

This code has been replaced to allow for missing white space.

> >+
> >+    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");

OK I changed it something like what you suggested.

> >+
> >+    /* Now we overwrite the first variable with the value of 0 or 1 */
> 
> magnus: ..overwrite with the value of the expression ?

Changed it to:

    /* Now we overwrite the first variable with 0 or 1 (for false or true) */

> >+    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.

I want a number to always compare unequal to a string, but without the
check for v2.is_int, 0 would compare *equal* to any string.

> >+    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 ?

No, bacause strcmp may return -1. OK I will rewrite to strcmp() != 0.

> >+
> >+let $ifvar= 5;
> >+let $ifvar2= 6;
> >+
> >+if ($ifvar<  7)
> 
> magnus:      ^ so no space required before cond?

Huh? There was a space there! But now it's no longer required.

> >+if (`SELECT 'something'`)
> >+{
> >+  echo anything goes;
> >+}
> >+
> 
> magnus: Add comment why test checks

OK.
 
- Bjorn