Skip to contentSkip to conversation

printf long format

PATCH In review Hollis Blanchard 3 participants 10 messages Jun 21, 2005 – Jun 22, 2005

lore
Share & tools
lore

Hollis Blanchard

author
#0 lore raw

PATCH 1/1 printf long format

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

misc.c +12 -3
diff -u -p -r1.19 misc.c
@@ -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));
#1 lore raw
Quoted 3 lines · Hollis Blanchard

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?

Quoted 2 lines

* 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.

misc.c grub_vsprintf (char *str, const char *fm · line 574

Quoted 9 lines
misc.c +1 -1
 	  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

author
#2 lore raw
Quoted 7 lines · Marco Gerards
Quoted 3 lines · Hollis Blanchard

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.

Quoted 5 lines
Quoted 2 lines

* 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...

Quoted 11 lines

misc.c grub_vsprintf (char *str, const char *fm · line 574

Quoted 9 lines
misc.c +1 -1
 	  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

#3 lore raw
Quoted 6 lines · Hollis Blanchard

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

#4 lore raw
Quoted 10 lines · Yoshinori K. Okuji
Quoted 6 lines · Hollis Blanchard

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

#5 lore raw
Quoted 2 lines · Marco Gerards

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

#6 lore raw
Quoted 10 lines · Yoshinori K. Okuji
Quoted 2 lines · Marco Gerards

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

#7 lore raw
Quoted 3 lines · Marco Gerards

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

#8 lore raw
Quoted 12 lines · Yoshinori K. Okuji
Quoted 3 lines · Marco Gerards

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:

Quoted 7 lines

/*
* 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

#9 lore raw
Quoted 1 line · Marco Gerards

Why not?

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

Quoted 1 line

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