From: Magnus BlĂ„udd
Date: November 9 2010 3:51pm
Subject: Re: bzr commit into mysql-5.5-mtr branch (bjorn.munch:3110) Bug#57276
List-Archive: http://lists.mysql.com/commits/123304
Message-Id: <4CD96E15.4000500@oracle.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=ISO-8859-1; format=flowed
Content-Transfer-Encoding: 8bit

Ok with me.

On 11/09/2010 04:30 PM, Bjorn Munch wrote:
> 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
>