List:Commits« Previous MessageNext Message »
From:Olav Sandstaa Date:November 16 2010 8:06pm
Subject:Re: bzr commit into mysql-5.5-bugteam branch (tor.didriksen:3123)
Bug#58137
View as plain text  
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
Thread
bzr commit into mysql-5.5-bugteam branch (tor.didriksen:3123) Bug#58137Tor Didriksen16 Nov
  • Re: bzr commit into mysql-5.5-bugteam branch (tor.didriksen:3123)Bug#58137Olav Sandstaa16 Nov