Re: [RFC,PATCH] C99 format specifiers for fixed-length integer types

[Pavel Roskin]

On Wed, 2009-07-22 at 21:10 +0200, Javier Martín wrote:
> This patch modifies the global types.h header to define a number of
> macros used for the formatted output of fixed-length integers like
> grub_uint64_t. Currently we use the traditional "%llu" format
> specifiers, adding casts as required to assuage the loud GCC.
> 
> However, casts to shut the compiler up are generally a bad idea, and the
> fact that this kind of output occurs mostly in debug code (printing an
> inode number, for example) which is dispersed about the GRUB codebase
> makes it very likely that the eventual breakage of one of these
> assumptions will result in either a stream of warnings/errors or, given
> that we are using casts to shut the compiler up, runtime weirdness.

I doubt about "runtime weirdness".  gcc is good at catching such
problems at the compile time.

> Using these C99-like format specifiers will allow us to have the
> relevant assumptions centralized in a single file, namely
> <grub/types.h>. Thus, if and when one is broken - for example, when
> sizeof(void*)!=sizeof(long), as it happens in mingw64 - the fix will be
> way simpler to develop and deploy.

It would be a good idea to use standard modifiers if they were not so
hard on the eye.

Also, the strictness it not increased by such change.  It's still
possible to use wrong modifiers and see nothing unless compiling for an
architecture where the size of the argument is different from the
expected size.

> For example, using a fictional MyFS:
> 
> typedef struct {
>     grub_uint64_t ino_num;
>     grub_uint16_t perm_flags;
> } myfs_ino_t;
> myfs_ino_t *cur = ...
> 
> The patch will turn something like this:
> 
>     grub_dprintf("myfs", "Inode number %llu has permissions %03o",
>                  (unsigned long) grub_be_to_cpu64(cur->ino_num),
>                  grub_be_to_cpu16(cur->perm_flags));
> 
> Into this:
> 
>     grub_dprintf("myfs", "Inode number %"GRUB_PRI64u" has permissions "
>                  "%03"GRUB_PRI16o, grub_be_to_cpu64(cur->ino_num),
>                  grub_be_to_cpu16(cur->perm_flags));

That's a biased example where the size of the arguments is obvious.  And
even then, it's hard to read.  And if you put GRUB_PRI32o instead of
GRUB_PRI16o there, the compiler won't tell you anything.

The patch includes the easy part, namely adding the macros.  But I
doesn't think it would be so pretty with the ugly part, that is,
"fixing" every almost every *printf call.  It will be very hard to
review.

> -#if GRUB_CPU_SIZEOF_VOID_P == 8
> +#if GRUB_CPU_SIZEOF_LONG == 8

This belongs to a separate patch.

-- 
Regards,
Pavel Roskin