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

[Chad MILLER]

Hi Jocelyn.

Great work!  It's very close to passing my review.  A few concerns:


On 26 Mar 2007, at 02:25, Jocelyn Fournier wrote:
> --- mysql-5.2-ref/strings/my_vsnprintf.c	2007-03-02  
> 19:24:42.000000000 +0100
> +++ mysql-5.2/strings/my_vsnprintf.c	2007-03-26 08:01:55.000000000  
> +0200
> @@ -17,6 +17,7 @@
>  #include <m_string.h>
>  #include <stdarg.h>
>  #include <m_ctype.h>
> +#include <decimal.h>
>
>  /*
>    Limited snprintf() implementations
> @@ -152,6 +153,34 @@
>        to+= res_length;
>        continue;
>      }
> +    else if (*fmt == 'f')   /* Float parameter */
> +    {
> +      register double larg;
> +      uint res_length, to_length;
> +      to_length= (uint) (end-to);
> +      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= malloc(sizeof(char)*to_length);

Use my_malloc() and my_free(), always.

> +      decimal.buf= (void*) buf1;
> +      decimal.len= (int) sizeof(buf1)/sizeof(decimal_digit_t);

Hmm, that division makes me uncomfortable.  I need tests before I  
trust that.  Intuitively, I suspect an off-by-one error.

> +      larg= va_arg(ap, double);
> +      /* converting the double format into decimal format */
> +      double2decimal(larg, &decimal);
> +      res_length= to_length;
> +      /* then converting the decimal into string */
> +      decimal2string(&decimal, to,
> +                     &res_length, /* max buffer size */
> +                                  /* will contain the size of the  
> result */
> +                     length ? length : (width ? (decimal.intg +  
> width) : 0), /* precision */
> +                     width, /* digits after the mantis */
> +                     0);

Add a note justifying the two steps -- just a few lines to the effect  
of "we want to gain the benefits of WL#2934 when it's completed."

> +      free(buf1);
> +      if (res_length > to_length)
> +        break;
> +      to+= res_length;
> +      continue;
> +    }
>      else if (*fmt == 'c')                       /* Character  
> parameter */
>      {
>        register int larg;
> --- mysql-5.2-ref/unittest/mysys/Makefile.am	2007-03-02  
> 19:24:43.000000000 +0100
> +++ mysql-5.2/unittest/mysys/Makefile.am	2007-03-23  
> 01:03:41.000000000 +0100
> @@ -17,9 +17,11 @@
>  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)/strings/libmystrings.a \
> +                  $(top_builddir)/dbug/libdbug.a \
>  		  $(top_builddir)/mysys/libmysys.a \
> -		  $(top_builddir)/dbug/libdbug.a \
> -		  $(top_builddir)/strings/libmystrings.a
> +                  $(top_builddir)/strings/libmystrings.a
>
>  noinst_PROGRAMS  = bitmap-t base64-t my_atomic-t

Yes!  You updated files in the unittest directory, but I don't see a  
test of the my_snprintf() .  Perhaps you added it, but the 'diff'  
program omitted it?

In particular, I'm interested in the results of:

char foo[20];
my_snprintf(foo, 10, ">%0f<", 0.987654321098765432);
my_snprintf(foo, 10, ">%.0f<", 0.987654321098765432);
my_snprintf(foo, 10, ">%09.4f<", 0.987654321098765432);
my_snprintf(foo, 10, ">%.1f<", 0.987654321098765432);
my_snprintf(foo, 3, ">%.1f<", 0.987654321098765432);
my_snprintf(foo, 4, ">%.1f<", 0.987654321098765432);
my_snprintf(foo, 10, ">%.3f<", 0.987654321098765432);
my_snprintf(foo, 10, ">%.4f<", 0.987654321098765432);
my_snprintf(foo, 10, ">%+.4f<", 0.987654321098765432);
my_snprintf(foo, 10, ">%+.5f<", 0.987654321098765432);
my_snprintf(foo, 10, ">%.6f<", 0.987654321098765432);
my_snprintf(foo, 10, ">%.7f<", 0.987654321098765432);
my_snprintf(foo, 10, ">%80f<", 0.987654321098765432);


Of course the test should fail if any results are not what we should  
expect.

Our test suite runs the programs in the "unittest" directory, but  
since I spend most of my time in the 5.0 tree, I don't offhand know  
how.  You may need to add a reference to it somewhere.  I suggest  
adding a bogus test and making sure that the normal suite fails and  
then adding the real tests and making sure that it passes.


> --- mysql-5.2-ref/libmysql/Makefile.shared	2007-03-02  
> 19:24:28.000000000 +0100
> +++ mysql-5.2/libmysql/Makefile.shared	2007-03-22  
> 23:57:59.000000000 +0100
> @@ -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.2-ref/libmysql/CMakeLists.txt	2007-03-02  
> 19:24:43.000000000 +0100
> +++ mysql-5.2/libmysql/CMakeLists.txt	2007-03-22 23:58:47.000000000  
> +0100
> @@ -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)
>
> --- mysql-5.2-ref/regex/Makefile.am	2007-03-02 19:24:43.000000000  
> +0100
> +++ mysql-5.2/regex/Makefile.am	2007-03-23 00:47:50.000000000 +0100
> @@ -17,7 +17,7 @@
>
>  INCLUDES =		-I$(top_builddir)/include -I$(top_srcdir)/include
>  noinst_LIBRARIES =	libregex.a
> -LDADD=			libregex.a $(top_builddir)/strings/libmystrings.a
> +LDADD=			libregex.a $(top_builddir)/strings/libmystrings.a

Not a big deal, but avoid changing whitespace at the end, if you can.

> $(top_builddir)/dbug/libdbug.a $(top_builddir)/mysys/libmysys.a $ 
> (top_builddir)/strings/libmystrings.a
>  noinst_HEADERS =	cclass.h cname.h regex2.h utils.h engine.c  
> my_regex.h
>  libregex_a_SOURCES =	regerror.c regcomp.c regexec.c regfree.c  
> reginit.c
>  noinst_PROGRAMS =	re

I guess the "status" command is too complex to include in the test  
suite.  :(  If it were in there, we never would have created this  
bug!  I wonder if the relatively recent "replace-regex" commands  
could permit it now.

- chad

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



Attachment: [application/pgp-signature] This is a digitally signed message part PGP.sig