[patch] printf long format

[patch] printf long format

[Hollis Blanchard]

Re: [patch] printf long format

[Marco Gerards]

Hollis Blanchard <hollis@penguinppc.org> writes:

You forgot your email...

--
Marco

[patch] printf long format

[Hollis Blanchard]

Hmm, no idea what happened to the first mail.

Debugging a partition map bug, I was adding more grub_dprintf messages...
and re-discovered that our printf doesn't handle e.g. "%lx" format strings (yet
gcc requires these when printing longs).

This patch works for me, though I didn't check that it implements all "l"
formating according to POSIX or SUS or whatever. Has not been tested on 64-bit
platforms. Comments?

-Hollis

	  * kern/misc.c (grub_vsprintf): Add `longfmt'.  If format string
	  contains `l' modifier, get a long from va_arg().

Index: kern/misc.c
===================================================================
RCS file: /cvsroot/grub/grub2/kern/misc.c,v
retrieving revision 1.19
diff -u -p -r1.19 misc.c
--- kern/misc.c	9 May 2005 01:47:37 -0000	1.19
+++ kern/misc.c	21 Jun 2005 04:02:35 -0000
@@ -562,13 +562,14 @@ grub_vsprintf (char *str, const char *fm
 	  char zerofill = ' ';
 	  int rightfill = 0;
 	  int n;
-	  
+	  int longfmt = 0;
+
 	  if (*fmt && *fmt =='-')
 	    {
 	      rightfill = 1;
 	      fmt++;
 	    }
-	  
+
 	  p = (char *) fmt;
 	  /* Read formatting parameters.  */
 	  while (*p && grub_isdigit (*p))
@@ -600,6 +601,11 @@ grub_vsprintf (char *str, const char *fm
 	    }
 
 	  c = *fmt++;
+	  if (c == 'l')
+	    {
+	      longfmt = 1;
+	      c = *fmt++;
+	    }
 
 	  switch (c)
 	    {
@@ -610,7 +616,10 @@ grub_vsprintf (char *str, const char *fm
 	    case 'x':
 	    case 'u':
 	    case 'd':
-	      n = va_arg (args, int);
+	      if (longfmt)
+		n = va_arg (args, long);
+	      else
+		n = va_arg (args, int);
 	      grub_itoa (tmp, c, n);
 	      if (!rightfill && grub_strlen (tmp) < format1)
 		write_fill (zerofill, format1 - grub_strlen (tmp));

Re: [patch] printf long format

[Marco Gerards]

Hollis Blanchard <hollis@penguinppc.org> writes:

> Debugging a partition map bug, I was adding more grub_dprintf messages...
> and re-discovered that our printf doesn't handle e.g. "%lx" format strings (yet
> gcc requires these when printing longs).

Isn't that hexadecimal?

> 	  * kern/misc.c (grub_vsprintf): Add `longfmt'.  If format string
> 	  contains `l' modifier, get a long from va_arg().

You forgot the first line...  Please make sure the date you use is the
date you check in.  With your previous commit something went wrong.

>  	  if (*fmt && *fmt =='-')
>  	    {
>  	      rightfill = 1;
>  	      fmt++;
>  	    }
> -	  
> +
>  	  p = (char *) fmt;
>  	  /* Read formatting parameters.  */

Please watch out for such stuff.  I know it is annoying...

The patch looks fine to me.

Thanks,
Marco

Re: [patch] printf long format

[Hollis Blanchard]

On Jun 22, 2005, at 6:05 AM, Marco Gerards wrote:
> Hollis Blanchard <hollis@penguinppc.org> writes:
>
>> Debugging a partition map bug, I was adding more grub_dprintf 
>> messages...
>> and re-discovered that our printf doesn't handle e.g. "%lx" format 
>> strings (yet
>> gcc requires these when printing longs).
>
> Isn't that hexadecimal?

Yes... but that's not the point.

>> 	  * kern/misc.c (grub_vsprintf): Add `longfmt'.  If format string
>> 	  contains `l' modifier, get a long from va_arg().
>
> You forgot the first line...  Please make sure the date you use is the
> date you check in.  With your previous commit something went wrong.

Sorry about that...

>>  	  if (*fmt && *fmt =='-')
>>  	    {
>>  	      rightfill = 1;
>>  	      fmt++;
>>  	    }
>> -	
>> +
>>  	  p = (char *) fmt;
>>  	  /* Read formatting parameters.  */
>
> Please watch out for such stuff.  I know it is annoying...

You have made this comment a few times now. Please understand: I do 
watch out for this stuff, apparently more carefully than other 
committers. I am removing whitespace from an otherwise blank line.

I am not saying I will never accidentally check in errant whitespace, 
but you can see that this is a correction, not an accident.

-Hollis

Re: [patch] printf long format

[Yoshinori K. Okuji]

On Wednesday 22 June 2005 16:33, Hollis Blanchard wrote:
> You have made this comment a few times now. Please understand: I do
> watch out for this stuff, apparently more carefully than other
> committers. I am removing whitespace from an otherwise blank line.
>
> I am not saying I will never accidentally check in errant whitespace,
> but you can see that this is a correction, not an accident.

Why does Marco complain about this? Because of the readability of a patch? In 
this case, the option --ignore-space-change to diff would help you...

Okuji

Re: [patch] printf long format

[Marco Gerards]

"Yoshinori K. Okuji" <okuji@enbug.org> writes:

> On Wednesday 22 June 2005 16:33, Hollis Blanchard wrote:
>> You have made this comment a few times now. Please understand: I do
>> watch out for this stuff, apparently more carefully than other
>> committers. I am removing whitespace from an otherwise blank line.
>>
>> I am not saying I will never accidentally check in errant whitespace,
>> but you can see that this is a correction, not an accident.
>
> Why does Marco complain about this? Because of the readability of a patch? In 
> this case, the option --ignore-space-change to diff would help you...

Readability.  Hollis prefers no whitespace on a line.  I do not care
about these spaces, but it is too much noise to either add or remove
it.

With --ignore-space-change you sometimes miss changes that were made
on purpose.

--
Marco

Re: [patch] printf long format

[Yoshinori K. Okuji]

On Wednesday 22 June 2005 18:24, Marco Gerards wrote:
> With --ignore-space-change you sometimes miss changes that were made
> on purpose.

OK, then how about --ignore-blank-lines?

Personally I do not care about including space changes in a patch. But if you 
really hate it, I believe that we should define a rule here, like always 
removing trailing spaces. Honestly speaking, I'd like to say, "Don't care too 
much".

Okuji

Re: [patch] printf long format

[Marco Gerards]

"Yoshinori K. Okuji" <okuji@enbug.org> writes:

> On Wednesday 22 June 2005 18:24, Marco Gerards wrote:
>> With --ignore-space-change you sometimes miss changes that were made
>> on purpose.
>
> OK, then how about --ignore-blank-lines?
>
> Personally I do not care about including space changes in a patch. But if you 
> really hate it, I believe that we should define a rule here, like always 
> removing trailing spaces. Honestly speaking, I'd like to say, "Don't care too 
> much".

In that case I will just stop nagging, unless blank lines are changed
in files that do not have other changes or when it makes a patch
really hard to read.  Is that fine for you?

Thanks,
Marco

Re: [patch] printf long format

[Yoshinori K. Okuji]

On Wednesday 22 June 2005 20:09, Marco Gerards wrote:
> In that case I will just stop nagging, unless blank lines are changed
> in files that do not have other changes or when it makes a patch
> really hard to read.  Is that fine for you?

No problem.

BTW, is there any way to have Emacs to remove trailing space when saving? As 
far as I know, this is possible with before_save_hook, but this requires 
modifying .emacs. Instead, I'd like to specify this within source files like 
this:

/*
 * Local variables:
 * delete-trailing-whitespace: t
 * End:
 */

I guess not possible at the moment...

Okuji

Re: [patch] printf long format

[Marco Gerards]

"Yoshinori K. Okuji" <okuji@enbug.org> writes:

> On Wednesday 22 June 2005 20:09, Marco Gerards wrote:
>> In that case I will just stop nagging, unless blank lines are changed
>> in files that do not have other changes or when it makes a patch
>> really hard to read.  Is that fine for you?
>
> No problem.
>
> BTW, is there any way to have Emacs to remove trailing space when saving? As 
> far as I know, this is possible with before_save_hook, but this requires 
> modifying .emacs. Instead, I'd like to specify this within source files like 
> this:
>
> /*
>  * Local variables:
>  * delete-trailing-whitespace: t
>  * End:
>  */
>
> I guess not possible at the moment...

Why not?

I don't have any objections to changing my .emacs either.

Thanks,
Marco

Re: [patch] printf long format

[Yoshinori K. Okuji]

On Wednesday 22 June 2005 22:14, Marco Gerards wrote:
> Why not?

Because I don't know how to do it. If you know, let me know.

> I don't have any objections to changing my .emacs either.

I do not want to change .emacs only for GRUB 2. Since .emacs is a global 
configuration, this affects other projects, and I do not like this.

Okuji