Hi Tor,
Patch looks good and solves the problem. OK to push as it is.
See inline for one minor comment (that you can feel free to ignore).
Tor Didriksen wrote:
> #At file:///export/home/didrik/repo/5.5-bugteam-bug58137/ based on
> revid:davi.arnaut@stripped
>
> 3123 Tor Didriksen 2010-11-16
> Bug #58137 char(0) column cause: my_gcvt: Assertion `width > 0 && to
> != ((void *)0)' failed
> @ mysql-test/r/func_math.result
> Add test for Bug #58137
> @ mysql-test/t/func_math.test
> Add test for Bug #58137
> @ sql/field.cc
> Skip calling my_gcvt() if we are trying to insert a double into a char(0)
> column.
>
> modified:
> mysql-test/r/func_math.result
> mysql-test/t/func_math.test
> sql/field.cc
> === modified file 'mysql-test/r/func_math.result'
> --- a/mysql-test/r/func_math.result 2010-10-08 09:52:09 +0000
> +++ b/mysql-test/r/func_math.result 2010-11-16 12:08:24 +0000
> @@ -607,3 +607,11 @@ SELECT floor(log10(format(concat_ws(5445
> as foo;
> foo
> 2
> +#
> +# Bug #58137 char(0) column cause: my_gcvt: Assertion `width > 0 && to !=
> ((void *)0)' failed
> +#
> +CREATE TABLE t1(a char(0));
> +INSERT INTO t1 (SELECT -pi());
> +Warnings:
> +Warning 1265 Data truncated for column 'a' at row 1
> +DROP TABLE t1;
>
> === modified file 'mysql-test/t/func_math.test'
> --- a/mysql-test/t/func_math.test 2010-10-08 09:52:09 +0000
> +++ b/mysql-test/t/func_math.test 2010-11-16 12:08:24 +0000
> @@ -464,3 +464,10 @@ SELECT -9223372036854775808 MOD -1;
> --echo #
> SELECT floor(log10(format(concat_ws(5445796E25, 5306463, 30837), -358821)))
> as foo;
> +
> +--echo #
> +--echo # Bug #58137 char(0) column cause: my_gcvt: Assertion `width > 0
> && to != ((void *)0)' failed
> +--echo #
> +CREATE TABLE t1(a char(0));
> +INSERT INTO t1 (SELECT -pi());
> +DROP TABLE t1;
>
> === modified file 'sql/field.cc'
> --- a/sql/field.cc 2010-10-29 09:35:07 +0000
> +++ b/sql/field.cc 2010-11-16 12:08:24 +0000
> @@ -6327,10 +6327,13 @@ int Field_str::store(double nr)
> ASSERT_COLUMN_MARKED_FOR_WRITE;
> char buff[DOUBLE_TO_STRING_CONVERSION_BUFFER_SIZE];
> uint local_char_length= field_length / charset()->mbmaxlen;
> - size_t length;
> - my_bool error;
> + size_t length= 0;
> + my_bool error= local_char_length == 0;
>
When first reading this it looked like an assignment. Would it make it
easier to read if you added parentheses around the boolean expression?
my_bool error= (local_char_length == 0);
I do not know if this makes it easier to read or not....
> +
> + // my_gcvt() requires width > 0, and we may have a CHAR(0) column.
> + if (!error)
> + length= my_gcvt(nr, MY_GCVT_ARG_DOUBLE, local_char_length, buff, &error);
>
An alternative to make both the if test more "natural" (testing on width
instead of error) and avoiding the boolean initialization expression:
my_bool error= TRUE;
// my_gcvt() requires width > 0, and we may have a CHAR(0) column.
if (local_char_length > 0)
length= my_gcvt(nr, MY_GCVT_ARG_DOUBLE, local_char_length, buff,
&error);
or:
my_bool error;
// my_gcvt() requires width > 0, and we may have a CHAR(0) column.
if (local_char_length > 0)
length= my_gcvt(nr, MY_GCVT_ARG_DOUBLE, local_char_length, buff,
&error);
else
error= TRUE;
Feel free to keep the original. I guess this is just a matter of taste...
Olav
