[prev in list] [next in list] [prev in thread] [next in thread]
List: mysql-internals
Subject: Re: Review of fourth patch for Bug#25615, "Status command doesn't
From: Jocelyn Fournier <joc () presence-pc ! com>
Date: 2007-04-10 19:26:02
Message-ID: 461BE4CA.9080701 () presence-pc ! com
[Download RAW message or body]
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@presence-pc.com 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
--
MySQL Internals Mailing List
For list archives: http://lists.mysql.com/internals
To unsubscribe: http://lists.mysql.com/internals?unsub=mysql-internals@progressive-comp.com
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic