Re: Review of fourth patch for Bug#25615, "Status command doesn't report anymore the Queries per second avg"

[Jocelyn Fournier]

Hi Chad,

Chad MILLER a écrit :
> Jocelyn,
> 
> Better!  This is okay with a few minor suggestions, below.
> 
> - chad
> 
> 
> On 9 Apr 2007, at 12:25, joce@stripped wrote:
> 
>> /* Copyright (C) 2007 MySQL AB
>>
>>    This program is free software; you can redistribute it and/or
>>    modify it under the terms of the GNU General Public License as
>>    published by the Free Software Foundation; version 2 of the License.
>>
>>    This program is distributed in the hope that it will be useful, but
>>    WITHOUT ANY WARRANTY; without even the implied warranty of
>>    MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
>>    General Public License for more details.
>>
>>    You should have received a copy of the GNU General Public License
>>    along with this program; if not, write to the Free Software
>>    Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 
>> 02111-1307  USA */
>>
>> #include <my_global.h>
>> #include <tap.h>
>> #include <m_string.h>
>>
>> int
>> main(void)
>> {
>>   char foo[20];
>>   my_snprintf(foo, 10, ">%0f<", 0.987654321098765432);
>>   ok(!strcmp(foo,">0.987654"), "foo value is %s, should be >0.987654", 
>> foo);
>>   my_snprintf(foo, 10, ">%.0f<", 0.987654321098765432);
>>   ok(!strcmp(foo,">1<"), "foo value is %s, should be >1<", foo);
>>   my_snprintf(foo, 10, ">%09.4f<", 0.987654321098765432);
>>   ok(!strcmp(foo,">0000.987"), "foo value is %s, should be >0000.987", 
>> foo);
>>   my_snprintf(foo, 10, ">%9.4f<", 0.987654321098765432);
>>   ok(!strcmp(foo,">   0.987"), "foo value is %s, should be >   0.987", 
>> foo);
>>   my_snprintf(foo, 10, ">%.1f<", 0.987654321098765432);
>>   ok(!strcmp(foo,">0.9<"), "foo value is %s, should be >0.9<", foo);
> 
> Huh.  Python gave me ">1.0<" for this.
> 
>>   my_snprintf(foo, 5, ">%.1f<", 0.987654321098765432);
>>   ok(!strcmp(foo,">0.9"), "foo value is %s, should be >0.9", foo);
>>   my_snprintf(foo, 4, ">%.1f<", 0.987654321098765432);
>>   ok(!strcmp(foo,">0."), "foo value is %s, should be >0.", foo);
>>   my_snprintf(foo, 10, ">%.3f<", 0.987654321098765432);
>>   ok(!strcmp(foo,">0.987<"), "foo value is %s, should be >0.987<",
> foo);
> 
> And ">0.988<" here.
> 
>>   my_snprintf(foo, 10, ">%.4f<", 0.987654321098765432);
>>   ok(!strcmp(foo,">0.9876<"), "foo value is %s, should be >0.9876<",
> 
>> foo);
> 
> And ">0.9877<" here.
> 
>>   my_snprintf(foo, 10, ">%+.4f<", 0.987654321098765432);
>>   ok(!strcmp(foo,">+0.9876<"), "foo value is %s, should be
> >+0.9876<", 
>> foo);
> 
> Rounded again.
> 
>>   my_snprintf(foo, 10, ">%+.4f<", -0.987654321098765432);
>>   ok(!strcmp(foo,">-0.9876<"), "foo value is %s, should be
> >-0.9876<", 
>> foo);
>>   my_snprintf(foo, 10, ">%+.5f<", 0.987654321098765432);
>>   ok(!strcmp(foo,">+0.98765"), "foo value is %s, should be >+0.98765", 
>> foo);
>>   my_snprintf(foo, 10, ">%.6f<", 0.987654321098765432);
>>   ok(!strcmp(foo,">0.987654"), "foo value is %s, should be >0.987654", 
>> foo);
>>   my_snprintf(foo, 10, ">%.7f<", 0.987654321098765432);
>>   ok(!strcmp(foo,">0.987654"), "foo value is %s, should be >0.987654", 
>> foo);
>>   my_snprintf(foo, 10, ">%80f<", 0.987654321098765432);
>>   ok(!strcmp(foo,">        "), "foo value is %s, should be >        ", 
>> foo);
>>   my_snprintf(foo, 10, ">%.3f<", 10.65464);
>>   ok(!strcmp(foo,">10.654<"), "foo value is %s, should be >10.654",
> foo);
> 
> And ">10.655<" here.
> 
> Okay, I checked this against my C reference and against my system.  
> """If the floating-point value cannot be represented exactly in the 
> number of digits produced, then the converted value should be the result 
> of rounding the exact floating-point value to the number of decimal 
> places produced."""
> 
> And my test yielded (Ubuntu Edgy):
>  >0.9877<
>  >+0.9877<
>  >10.655<
> 
> The reference says "should", not "must", but I wonder if we can close 
> the distance and make them behave the same.  Any ideas, Jocelyn?
> 

Yep I noticed the problem too, but the current implementation of 
decimal2string doesn't do any rounding, it just truncates the result the 
the precision char, so I assumed the results was ok until the #WL on 
floating number is done.


> This is probably a problem elsewhere, not in your code.  If it's 
> system-dependent, then these tests aren't reliable enough to go into the 
> test suite and I'll have to change them.
> 
> You needn't worry about the above, but if you have suggestions, do speak 
> up.
> 
>>   return exit_status();
>> }
> 
> New Makefile.am file for tests/(something)?  What's the name?

unittest/strings/

> 
>> # Copyright (C) 2006 MySQL AB
>> #
>> # This program is free software; you can redistribute it and/or modify
>> # it under the terms of the GNU General Public License as published by
>> # the Free Software Foundation; version 2 of the License.
>> #
>> # This program is distributed in the hope that it will be useful,
>> # but WITHOUT ANY WARRANTY; without even the implied warranty of
>> # MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> # GNU General Public License for more details.
>> #
>> # You should have received a copy of the GNU General Public License
>> # along with this program; if not, write to the Free Software
>> # Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  
>> 02110-1301  USA
>>
>> AM_CPPFLAGS      = @ZLIB_INCLUDES@ -I$(top_builddir)/include
>> AM_CPPFLAGS     += -I$(top_srcdir)/include -I$(top_srcdir)/unittest/mytap
>>
>> LDADD         = $(top_builddir)/unittest/mytap/libmytap.a \
>>                       $(top_builddir)/mysys/libmysys.a \
>>             $(top_builddir)/dbug/libdbug.a \
>>             $(top_builddir)/strings/libmystrings.a
>>
>> noinst_PROGRAMS  = decimal-t
>>
>> --- mysql-5.1-ref/strings/my_vsnprintf.c    2007-01-22 
>> 13:10:43.000000000 +0100
>> +++ mysql-5.1/strings/my_vsnprintf.c    2007-04-09 18:07:59.000000000 
>> +0200
>> @@ -14,9 +14,11 @@
>>     Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  
>> 02111-1307  USA */
>>
>>  #include <my_global.h>
>> +#include <my_sys.h>
>>  #include <m_string.h>
>>  #include <stdarg.h>
>>  #include <m_ctype.h>
>> +#include <decimal.h>
>>
>>  /*
>>    Limited snprintf() implementations
>> @@ -33,9 +35,12 @@
>>      %#[l]d
>>      %#[l]u
>>      %#[l]x
>> +    %#f
>>      %#.#b     Local format; note first # is ignored and second is 
>> REQUIRED
>>      %#.#s    Note first # is ignored
>> -
>> +    %[+][#][#][.#]f
>> +
>> +
>>    RETURN
>>      length of result string
>>  */
>> @@ -43,32 +48,38 @@
>>  int my_vsnprintf(char *to, size_t n, const char* fmt, va_list ap)
>>  {
>>    char *start=to, *end=to+n-1;
>> -  uint length, width, pre_zero, have_long;
>> -
>> +  uint width, precision, pre_zero, have_long, expected_size, 
>> add_sign= 0;
>>    for (; *fmt ; fmt++)
>>    {
>>      if (*fmt != '%')
>>      {
>>        if (to == end)            /* End of buffer */
>> -    break;
>> +        break;
> 
> Heh, thank you.

;)

> 
>>        *to++= *fmt;            /* Copy ordinary char */
>>        continue;
>>      }
>>      fmt++;                    /* skip '%' */
>> -    /* Read max fill size (only used with %d and %u) */
>> +    /* Skip - char, not supported */
>>      if (*fmt == '-')
>>        fmt++;
>> -    length= width= pre_zero= have_long= 0;
>> +    if (*fmt == '+')
>> +    {
>> +      add_sign= 1;
>> +      fmt++;
>> +    }
>> +
>> +    precision= width= pre_zero= have_long= 0;
>> +    /* Read max fill size (only used with %d and %u) */
>>      if (*fmt == '*')
>>      {
>>        fmt++;
>> -      length= va_arg(ap, int);
>> +      width= va_arg(ap, int);
>>      }
>>      else
>>        for (; my_isdigit(&my_charset_latin1, *fmt); fmt++)
>>        {
>> -        length= length * 10 + (uint)(*fmt - '0');
>> -        if (!length)
>> +        width= width * 10 + (uint)(*fmt - '0');
>> +        if (!width)
>>            pre_zero= 1;            /* first digit was 0 */
>>        }
>>      if (*fmt == '.')
>> @@ -77,14 +88,14 @@
>>        if (*fmt == '*')
>>        {
>>          fmt++;
>> -        width= va_arg(ap, int);
>> +        precision= va_arg(ap, int);
>>        }
>>        else
>>          for (; my_isdigit(&my_charset_latin1, *fmt); fmt++)
>> -          width= width * 10 + (uint)(*fmt - '0');
>> +          precision= precision * 10 + (uint)(*fmt - '0');
>>      }
>>      else
>> -      width= ~0;
>> +      precision= ~0;
> 
> Means "none set", yes?

Yes.

> 
>>      if (*fmt == 'l')
>>      {
>>        fmt++;
>> @@ -96,9 +107,9 @@
>>        uint plen,left_len = (uint)(end-to)+1;
>>        if (!par) par = (char*)"(null)";
>>        plen = (uint) strlen(par);
>> -      set_if_smaller(plen,width);
>> +      set_if_smaller(plen,precision);
>>        if (left_len <= plen)
>> -    plen = left_len - 1;
>> +        plen = left_len - 1;
>>        to=strnmov(to,par,plen);
>>        continue;
>>      }
>> @@ -106,10 +117,10 @@
>>      {
>>        char *par = va_arg(ap, char *);
>>        DBUG_ASSERT(to <= end);
>> -      if (to + abs(width) + 1 > end)
>> -        width= end - to - 1;  /* sign doesn't matter */
>> -      memmove(to, par, abs(width));
>> -      to+= width;
>> +      if (to + abs(precision) + 1 > end)
>> +        precision= end - to - 1;  /* sign doesn't matter */
>> +      memmove(to, par, abs(precision));
>> +      to+= precision;
>>        continue;
>>      }
>>      else if (*fmt == 'd' || *fmt == 'u'|| *fmt== 'x')    /* Integer 
>> parameter */
>> @@ -119,8 +130,8 @@
>>        char *store_start= to, *store_end;
>>        char buff[32];
>>
>> -      if ((to_length= (uint) (end-to)) < 16 || length)
>> -    store_start= buff;
>> +      if ((to_length= (uint) (end-to)) < 16 || width)
>> +        store_start= buff;
>>        if (have_long)
>>          larg = va_arg(ap, long);
>>        else
>> @@ -129,29 +140,100 @@
>>          else
>>            larg= (long) (uint) va_arg(ap, int);
>>        if (*fmt == 'd')
>> -    store_end= int10_to_str(larg, store_start, -10);
>> +        store_end= int10_to_str(larg, store_start, -10);
>>        else
>>          if (*fmt== 'u')
>>            store_end= int10_to_str(larg, store_start, 10);
>>          else
>>            store_end= int2str(larg, store_start, 16, 0);
>>        if ((res_length= (uint) (store_end - store_start)) > to_length)
>> -    break;                    /* num doesn't fit in output */
>> +        break;                    /* num doesn't fit in output */
>>        /* If %#d syntax was used, we have to pre-zero/pre-space the 
>> string */
>>        if (store_start == buff)
>>        {
>> -    length= min(length, to_length);
>> -    if (res_length < length)
>> -    {
>> -      uint diff= (length- res_length);
>> -      bfill(to, diff, pre_zero ? '0' : ' ');
>> -      to+= diff;
>> -    }
>> -    bmove(to, store_start, res_length);
>> +        width= min(width, to_length);
>> +        if (res_length < width)
>> +        {
>> +          uint diff= (width- res_length);
> 
> Ugly subtraction there.  Not a big deal.
> 
>> +          bfill(to, diff, pre_zero ? '0' : ' ');
>> +          to+= diff;
>> +        }
>> +        bmove(to, store_start, res_length);
>>        }
>>        to+= res_length;
>>        continue;
>>      }
>> +    else if (*fmt == 'f')   /* Float parameter */
>> +    {
>> +      register double larg;
>> +      uint res_length, to_length;
>> +      larg= va_arg(ap, double);
>> +      if (add_sign == 1)
>> +      {
>> +        if (larg >= 0.0)
>> +          *to++= '+';
>> +      }
>> +      to_length= (uint) (end-to);
>> +      /* handle the .0f case */
>> +      if (precision == 0)
>> +      {
>> +        larg= (double) ceil(larg);
>> +      }
>> +      decimal_t decimal;
>> +      /* decimal buf and len fields must be set before calling 
>> double2decimal */
>> +      /* allocating a buffer of the size of the remaining space in 
>> "to" buffer */
>> +      char* buf1= 
>> my_alloca(ceil(sizeof(char)*to_length/sizeof(decimal_digit_t))
>> +                            *sizeof(decimal_digit_t));
>> +      decimal.buf= (void*) buf1;
>> +      decimal.len= sizeof(buf1)/sizeof(decimal_digit_t);
>> +      /* we are currently implementing %f using combination of
>> +         double2decimal and decimal2double. We could gain the 
>> benefits of
>                                       ^ string
> 
>> +         WL#2934 once it's completed */
> 
> The comments don't conform to our style.
> 

One /* */ by line ?

Thanks,
   Jocelyn