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
