Re: bk commit into 5.0 tree (tnurnberg:1.2526) BUG#31227

[Georgi Kodinov]

Hi,

Basically the same remark as for 31253 : very good analysis; good fix  
that is just not communicated well.

On 04.10.2007, at 08:49, Tatjana A Nuernberg wrote:

> Below is the list of changes that have just been committed into a  
> local
> 5.0 repository of tnurnberg. When tnurnberg does a push these  
> changes will
> be propagated to the main repository and, within 24 hours after the
> push, to the public repository.
> For information on how to access the public repository
> see http://dev.mysql.com/doc/mysql/en/installing-source-tree.html
>
> ChangeSet@stripped, 2007-10-04 07:49:08+02:00,  
> tnurnberg@stripped +3 -0
>   Bug#31227: memory overrun with decimal (6,6) and zerofill and  
> group_concat
>
>   This is a pathological case.
>   DECIMAL(a,b) == a digits altogether, of which b are fractional.
>   DECIMAL(5,3)    123.45, or -123.45(!)
>
>   This requires a characters,
>   plus one extra for sign,
>   plus one for decimal point if b>0,
>   *plus* one extra for a leading '0' if a==b:
>
>   DECIMAL(6,6)  -0.123456:
>   (sign) (zero) (point) (6 decimal places) (\0)
>
>   This leading '0' was not factored in.

This would fit ideally to the function comment.
If you insist on having it as a commit comment please add: "Fixed  
by ...." (counting the leading '0' when it's the only digit before  
the decimal point).

>   sql/my_decimal.cc@stripped, 2007-10-04 07:49:03+02:00,  
> tnurnberg@stripped +3 -1
>     Even if the user only requests decimal places for
>     DECIMAL (as in DECIMAL(a,a)), we'll print a leading
>     zero before the decimal point. Make sure we allocate
>     one extra character for the '0' in such cases.

This would be the ideal commit comment : concise and up to the  
point : also contains the fix description.

<snip>
> diff -Nrup a/sql/my_decimal.cc b/sql/my_decimal.cc
> --- a/sql/my_decimal.cc	2007-05-16 10:44:40 +02:00
> +++ b/sql/my_decimal.cc	2007-10-04 07:49:03 +02:00
> @@ -85,7 +85,9 @@ int my_decimal2string(uint mask, const m
>                        uint fixed_prec, uint fixed_dec,
>                        char filler, String *str)
>  {
> -  int length= (fixed_prec ? (fixed_prec + 1) :  
> my_decimal_string_length(d));
> +  int length= (fixed_prec
> +               ? (fixed_prec + ((fixed_prec == fixed_dec) ? 1 : 0)  
> + 1)
> +               : my_decimal_string_length(d));
>    int result;
>    if (str->alloc(length))
>      return check_result(mask, E_DEC_OOM);

Since you are changing more than 30% of the function's code, please  
add a function doxygenized comment (according to http:// 
forge.mysql.com/wiki/MySQL_Internals_Coding_Guidelines, "Commenting  
code").
Also please make sure all the +1 are commented. I'd add comment as  
follows :
fixed_prec ?
   fixed_prec +
   ((fixed_prec == fixed_dec) ? 1 : 0) // Add 1 if there's only a  
leading zero before the decimal point
   + 1                                 // add 1 for the decimal  
point ? or for the sign ?
   ...
See : it's good to have comments :-)
Please extend your test to cover -0.[0-9] cases and think of what you  
need to add exactly : IMHO you'll need to add 1 more in this case.

Best Regards,
Joro
-- 
Georgi Kodinov, Senior Software Engineer
MySQL AB, Plovdiv, Bulgaria, www.mysql.com
Office: [phone] Mobile: [phone] Skype: georgekodinov

Are you MySQL certified?  www.mysql.com/certification