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

[Jorge Bernal Ordovás]

OK, here's the second version. It seems Apple Mail sends .patch files  
as binary attachments, so I guess that's the reason for being filtered.

Just in case it don't make it to the list:
http://people.warp.es/~koke/patches/27894-fix-printf_2.diff

On 19/04/2007, at 11:04, Chad MILLER wrote:
>
> Be sure not to store tab characters in the source.
>
> Also, "bool" is a C++ type.  Use "my_bool", which we define locally.
>
Done, I wasn't sure if we had bool, so I grep'd the code, but didn't  
notice that part would be in C++

>> +		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.
>
I've basically set a fixed size for buffz. It could be another way to  
do this (in fact, there are lots of ways), but I don't see how to do  
that without turning the code in something too comples.

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

Thanks for the review
	Koke


--- 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-19 13:32:13.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 */
+  my_bool is_zero_padded;
 
   /*
     Store the location of the beginning of a format directive, for the
@@ -337,8 +338,10 @@
     minimum_width= 0;
     precision= 0;
     minimum_width_sign= 1;
+    is_zero_padded= FALSE;
     /* Skip if max size is used (to be compatible with printf) */
     while (*fmt == '-') { fmt++; minimum_width_sign= -1; }
+    while (*fmt == '0') { fmt++; is_zero_padded= TRUE; }
     if (*fmt == '*') {
       precision= (int) va_arg(args, int);
       fmt++;
@@ -390,6 +393,15 @@
 	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) 
+	{
+		char buffz[17];
+		if (is_zero_padded)
+			memset(buffz, '0', minimum_width - length2);
+		else
+			memset(buffz, ' ', minimum_width - length2);
+		my_b_write(info, buffz, minimum_width - length2);
+	}
       out_length+= length2;
       if (my_b_write(info, buff, length2))
 	goto err;


--
Jorge Bernal Ordovás  <jbernal@stripped>
http://amedias.org/ [ES]
http://koke.amedias.org/ [EN]

Warp Networks         http://www.warp.es/
D. Jaime I 33 3º Dcha, 50001 Zaragoza, Spain