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

[Chad MILLER]

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?

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?

> # 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?

>      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.

> +      /* converting the double format into decimal format */
> +      double2decimal(larg, &decimal);
> +      res_length= to_length+1; /* res_length include the trailing  
> \0 */
> +      /* if no precision is specified,
> +         set the precision to the number of digit after the mantis */
> +      if (precision == (uint) ~0)
> +      {
> +        precision= decimal.frac;
> +      }
> +      /* width include sign and decimal point */
> +      if (precision)
> +      {
> +        expected_size= (decimal.intg + test(decimal.frac)
> +                        + decimal.sign + precision);
> +      }
> +      else
> +      {
> +        expected_size= 0;
> +      }
> +      width= width ? width : (precision ? expected_size : 0);
> +      /* if the width is shorter than the number of characters to  
> be printed,
> +         no truncation */
> +      if (width != 0 && (width < expected_size))
> +      {
> +        width= expected_size;
> +      }
> +      /* width value passed to decimal2string should not include  
> sign and
> +         decimal point */
> +      if (width != 0)
> +      {
> +        width= width - decimal.sign - test(decimal.frac);
> +      }
> +      /* converting the decimal into string */
> +      decimal2string(&decimal, to,
> +                     &res_length, /* max buffer size */
> +                     /* will contain the size of the result */
> +                     width, /* minimum number of characters to be  
> printed */
> +                     precision, /* number of digits after the  
> mantis */
> +                     pre_zero?'0':' ' /* pad with zero if required  
> */);
> +      my_afree(buf1);
> +      if (res_length > to_length)
> +        res_length= to_length;
> +      to+= res_length;
> +      continue;
> +    }

Okay.

>      else if (*fmt == 'c')                       /* Character  
> parameter */
>      {
>        register int larg;
> --- mysql-5.1-ref/libmysql/Makefile.shared	2007-04-02  
> 10:37:24.000000000 +0200
> +++ mysql-5.1/libmysql/Makefile.shared	2007-04-08  
> 01:16:41.000000000 +0200
> @@ -46,7 +46,7 @@
>  			ctype-win1250ch.lo ctype-utf8.lo ctype-extra.lo \
>  			ctype-ucs2.lo ctype-gb2312.lo ctype-gbk.lo \
>  			ctype-sjis.lo ctype-tis620.lo ctype-ujis.lo \
> -			ctype-uca.lo xml.lo my_strtoll10.lo str_alloc.lo
> +			ctype-uca.lo xml.lo my_strtoll10.lo str_alloc.lo decimal.lo
>
>  mystringsextra= 	strto.c
>  dbugobjects =		dbug.lo # IT IS IN SAFEMALLOC.C sanity.lo
> --- mysql-5.1-ref/libmysql/CMakeLists.txt	2007-04-02  
> 10:37:32.000000000 +0200
> +++ mysql-5.1/libmysql/CMakeLists.txt	2007-04-08 01:17:52.000000000  
> +0200
> @@ -61,7 +61,7 @@
>                       ../strings/strmov.c ../strings/strnlen.c ../ 
> strings/strnmov.c ../strings/strtod.c
>                       ../strings/strtoll.c ../strings/strtoull.c ../ 
> strings/strxmov.c ../strings/strxnmov.c
>                       ../mysys/thr_mutex.c ../mysys/typelib.c ../ 
> vio/vio.c ../vio/viosocket.c
> -                     ../vio/viossl.c ../vio/viosslfactories.c ../ 
> strings/xml.c)
> +                     ../vio/viossl.c ../vio/viosslfactories.c ../ 
> strings/xml.c ../strings/decimal.c)
>  ADD_DEPENDENCIES(libmysql dbug vio mysys strings GenError zlib  
> yassl taocrypt)
>  TARGET_LINK_LIBRARIES(libmysql mysys strings wsock32)
>

Okay.

> --- mysql-5.1-ref/strings/decimal.c	2007-02-23 12:23:43.000000000  
> +0100
> +++ mysql-5.1/strings/decimal.c	2007-04-09 18:21:21.000000000 +0200
> @@ -374,18 +374,28 @@
>        intg= fixed_intg;
>      }
>    }
> -  else if (unlikely(len > --*to_len)) /* reserve one byte for \0 */
> +  /* len and *to_len both takes into account \0 */
> +  if (unlikely(len > *to_len))
>    {
>      int j= len-*to_len;
>      error= (frac && j <= frac + 1) ? E_DEC_TRUNCATED :  
> E_DEC_OVERFLOW;
>      if (frac && j >= frac + 1) j--;
>      if (j > frac)
>      {
> -      intg-= j-frac;
> -      frac= 0;
> +      if (intg > (j-frac))
> +      {
> +        intg-= j-frac;
> +        frac= 0;
> +      }
> +      else
> +      {
> +        intg= 0;
> +        intg_len= *to_len - from->sign + 1;
> +        frac= 0;
> +      }
>      }
>      else
> -      frac-=j;
> +      frac-= j;
>      len= from->sign + intg_len + test(frac) + frac_len;
>    }
>    *to_len=len;
> @@ -964,14 +974,14 @@
>      exp+= DIG_PER_DEC1;
>    }
>
> -  DBUG_PRINT("info", ("interm.: %f %d %f", result, exp,
> -             scaler10[exp / 10] * scaler1[exp % 10]));
> +  /* DBUG_PRINT("info", ("interm.: %f %d %f", result, exp,
> +             scaler10[exp / 10] * scaler1[exp % 10])); */
>
>    result/= scaler10[exp / 10] * scaler1[exp % 10];
>
>    *to= from->sign ? -result : result;
>
> -  DBUG_PRINT("info", ("result: %f (%lx)", *to, *(ulong *)to));
> +  /* DBUG_PRINT("info", ("result: %f (%lx)", *to, *(ulong *)to)); */
>
>    return E_DEC_OK;
>  }
> @@ -993,13 +1003,14 @@
>    /* TODO: fix it, when we'll have dtoa */
>    char buff[400], *end;
>    int length, res;
> -  DBUG_ENTER("double2decimal");
> +  /* DBUG_ENTER("double2decimal"); */
>    length= my_sprintf(buff, (buff, "%.16G", from));
> -  DBUG_PRINT("info",("from: %g  from_as_str: %s", from, buff));
> +  /* DBUG_PRINT("info",("from: %g  from_as_str: %s", from, buff)); */
>    end= buff+length;
>    res= string2decimal(buff, to, &end);
> -  DBUG_PRINT("exit", ("res: %d", res));
> -  DBUG_RETURN(res);
> +  /* DBUG_PRINT("exit", ("res: %d", res));
> +  DBUG_RETURN(res); */
> +  return res;
>  }
>
>
> --- mysql-5.1-ref/configure.in	2007-03-28 23:03:31.000000000 +0200
> +++ mysql-5.1/configure.in	2007-04-09 04:00:04.000000000 +0200
> @@ -2547,6 +2547,7 @@
>  AC_CONFIG_FILES(Makefile extra/Makefile mysys/Makefile dnl
>   unittest/Makefile unittest/mytap/Makefile unittest/mytap/t/ 
> Makefile dnl
>   unittest/mysys/Makefile unittest/examples/Makefile dnl
> + unittest/strings/Makefile dnl
>   strings/Makefile regex/Makefile storage/Makefile dnl
>   man/Makefile BUILD/Makefile vio/Makefile dnl
>   libmysql/Makefile client/Makefile dnl
> --- mysql-5.1-ref/unittest/Makefile.am	2007-04-02  
> 10:37:21.000000000 +0200
> +++ mysql-5.1/unittest/Makefile.am	2007-04-09 03:57:19.000000000 +0200
> @@ -13,12 +13,12 @@
>  # along with this program; if not, write to the Free Software
>  # Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA   
> 02110-1301  USA
>
> -SUBDIRS      = mytap . mysys examples
> +SUBDIRS      = mytap . mysys strings examples
>
>  EXTRA_DIST = unit.pl
>  CLEANFILES = unit
>
> -unittests = mytap mysys @mysql_se_unittest_dirs@   
> @mysql_pg_unittest_dirs@
> +unittests = mytap mysys strings @mysql_se_unittest_dirs@   
> @mysql_pg_unittest_dirs@
>
>  test:
>  	perl unit.pl run $(unittests)

--
Chad Miller, Software Developer                         chad@stripped
MySQL Inc., www.mysql.com
Orlando, Florida, USA                                13-20z,  UTC-0400
Office: [phone]                         sip:6740@stripped