List:Commits« Previous MessageNext Message »
From:Martin Hansson Date:March 22 2010 9:55am
Subject:Re: bzr commit into mysql-5.1-bugteam branch (joro:3404) Bug#51850
View as plain text  
Hi,
Patch approved but some comments on coding.

>
> === modified file 'sql/item.cc'
> --- a/sql/item.cc	2010-03-15 12:07:16 +0000
> +++ b/sql/item.cc	2010-03-18 15:30:33 +0000
> @@ -5068,6 +5068,17 @@ int Item_field::save_in_field(Field *to,
>     else
>     {
>       to->set_notnull();
> +
> +    /*
> +      If we're setting the same field as the one we're reading from there's
> +      nothing to do. This can happen in 'SET x = x' type of scenarios.
> +      */
> +    if (to == result_field)
> +    {
> +      null_value=0;
> +      return 0;
> +    }
> +
>       res= field_conv(to,result_field);
>       null_value=0;
>     }

Nesting if-statements generally makes code harder to follow. In this 
case the function is more difficult to follow after applying the patch. 
This is not what our code base needs. I don't see any need for nesting 
like this, except to keep a single point of exit, which personally, I 
don't see the point of. IMHO it is more appropriate to structure the 
code like

int Item_field::save_in_field(Field *to, bool no_conversions)
{
   if (result_field->is_null())
   {
     null_value=1;
     return set_field_to_null_with_conversions(to, no_conversions);
   }
   to->set_notnull();

   /*
     If we're setting the same field as the one we're reading from there's
     nothing to do. This can happen in 'SET x = x' type of scenarios.
   */
   if (to == result_field)
   {
     null_value= 0;
     return 0;
   }

   int res= field_conv(to,result_field);
   null_value=0;
   return res;
}

But it's your fix, so it's your call.

Best Regards,

Martin
Thread
bzr commit into mysql-5.1-bugteam branch (joro:3404) Bug#51850Georgi Kodinov18 Mar
  • Re: bzr commit into mysql-5.1-bugteam branch (joro:3404) Bug#51850Martin Hansson22 Mar
    • Re: bzr commit into mysql-5.1-bugteam branch (joro:3404) Bug#51850Georgi Kodinov24 Mar