Review 1 of patch 1 for #27894: mysqlbinlog formats timestamp wrong in comment

[Chad MILLER]

On 19 Apr 2007, at 12:16, Jorge Bernal Ordovás wrote:
> On 19/04/2007, at 7:49, Chad MILLER wrote:
>> On 19 Apr 2007, at 00:40, Jorge Bernal wrote:
>>> here goes a patch for the bug <http://bugs.mysqsl.com/27894>.
>>>
>>> I have to warn that it's my first approach to MySQL so it might  
>>> be wrong, or could be more efficient, but it works as a starting  
>>> point.
>>>
>>> I'd appreciate any comments/suggestions
>>
>> Hi Koke.  Did you forget to attach the patch?  I don't see it.
>
> I remember seeing the attached file, but it's not there, so here it  
> goes


Thank you for the patch, Koke.  I would like this bug fixed.

Here's my review of your patch.


> --- mysql-5.1-trunk/mysys/mf_iocache2.c	2007-02-24  
> 02:43:10.000000000 -0800
> +++ mysql-5.1/mysys/mf_iocache2.c	2007-04-18 21:31:36.000000000 -0700
> @@ -299,6 +299,7 @@
>    uint minimum_width; /* as yet unimplemented */
>    uint minimum_width_sign;
>    uint precision; /* as yet unimplemented for anything but %b */
> +	bool zero_padding;

Be sure not to store tab characters in the source.

Also, "bool" is a C++ type.  Use "my_bool", which we define locally.

It may be nest to name this like a predicate, since it answers a yes/ 
no question and isn't the amount of padding.  Maybe "is_zero_padded" ?

>    /*
>      Store the location of the beginning of a format directive, for  
> the
> @@ -337,6 +338,7 @@
>      minimum_width= 0;
>      precision= 0;
>      minimum_width_sign= 1;
> +    zero_padding= FALSE;
>      /* Skip if max size is used (to be compatible with printf) */
>      while (*fmt == '-') { fmt++; minimum_width_sign= -1; }
>      if (*fmt == '*') {
> @@ -344,7 +346,10 @@
>        fmt++;
>      } else {
>        while (my_isdigit(&my_charset_latin1, *fmt)) {
> -        minimum_width=(minimum_width * 10) + (*fmt - '0');
> +        if ((*fmt == '0') && (zero_padding == FALSE))
> +					zero_padding= TRUE;

Should that be inside the "while"?  Consider the number "100".  Only  
leading zeroes should trigger this, yes?  I would make it like the  
"-" condition above.

> +				else
> +	        minimum_width=(minimum_width * 10) + (*fmt - '0');

Please try to avoid removing lines and re-adding them.

>          fmt++;
>        }
>      }
> @@ -390,6 +395,14 @@
> 	length2= (uint) (int10_to_str((long) iarg,buff, -10) - buff);
>        else
> 	length2= (uint) (int10_to_str((long) (uint) iarg,buff,10)- buff);
> +	if (minimum_width > length2) {

Our style guide says the curly-brace should be on the next line,  
beneath the start of "if".

> +		char buffz[minimum_width - length2];

I don't think that's portable.  The size of the array should be a  
constant.

> +		if (zero_padding)
> +			memset(buffz, '0', minimum_width - length2);
> +		else
> +			memset(buffz, ' ', minimum_width - length2);
> +		my_b_write(info, buffz, minimum_width - length2);

In light of the constancy of the "buffz" size, perhaps you can think  
of a better way to do that.

> +	}
>        out_length+= length2;
>        if (my_b_write(info, buff, length2))
> 	goto err;


Thank you.  This is a good first effort.  If you fix these  
complaints, I'll eagerly add it to our source.

- 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