* [RFC,PATCH] C99 format specifiers for fixed-length integer types
@ 2009-07-22 19:10 Javier Martín
2009-07-23 1:08 ` Pavel Roskin
2009-07-23 18:54 ` Javier Martín
0 siblings, 2 replies; 11+ messages in thread
From: Javier Martín @ 2009-07-22 19:10 UTC (permalink / raw)
To: The development of GRUB 2
[-- Attachment #1.1: Type: text/plain, Size: 1653 bytes --]
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.
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.
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));
--
-- Lazy, Oblivious, Recurrent Disaster -- Habbit
[-- Attachment #1.2: prifmtspecs.patch --]
[-- Type: text/x-patch, Size: 1902 bytes --]
Index: include/grub/types.h
===================================================================
--- include/grub/types.h (revision 2438)
+++ include/grub/types.h (working copy)
@@ -58,23 +58,53 @@
# endif
#endif
-/* Define various wide integers. */
+/* Define various wide integers and their format specifiers. */
typedef signed char grub_int8_t;
typedef short grub_int16_t;
typedef int grub_int32_t;
-#if GRUB_CPU_SIZEOF_VOID_P == 8
+#define GRUB_PRIi8 i
+#define GRUB_PRIi16 i
+#define GRUB_PRIi32 i
+#define GRUB_PRId8 d
+#define GRUB_PRId16 d
+#define GRUB_PRId32 d
+#if GRUB_CPU_SIZEOF_LONG == 8
typedef long grub_int64_t;
+#define GRUB_PRIi64 li
+#define GRUB_PRId64 ld
#else
typedef long long grub_int64_t;
+#define GRUB_PRIi64 lli
+#define GRUB_PRId64 lld
#endif
typedef unsigned char grub_uint8_t;
typedef unsigned short grub_uint16_t;
typedef unsigned grub_uint32_t;
-#if GRUB_CPU_SIZEOF_VOID_P == 8
+#define GRUB_PRIu8 u
+#define GRUB_PRIu16 u
+#define GRUB_PRIu32 u
+#define GRUB_PRIo8 o
+#define GRUB_PRIo16 o
+#define GRUB_PRIo32 o
+#define GRUB_PRIx8 x
+#define GRUB_PRIx16 x
+#define GRUB_PRIx32 x
+#define GRUB_PRIX8 X
+#define GRUB_PRIX16 X
+#define GRUB_PRIX32 X
+#if GRUB_CPU_SIZEOF_LONG == 8
typedef unsigned long grub_uint64_t;
+#define GRUB_PRIu64 lu
+#define GRUB_PRIo64 lo
+#define GRUB_PRIx64 lx
+#define GRUB_PRIX64 lX
#else
typedef unsigned long long grub_uint64_t;
+#define GRUB_PRIu64 llu
+#define GRUB_PRIo64 llo
+#define GRUB_PRIx64 llx
+#define GRUB_PRIX64 llX
#endif
/* Misc types. */
@@ -100,7 +130,7 @@
typedef grub_int32_t grub_ssize_t;
#endif
-#if GRUB_CPU_SIZEOF_VOID_P == 8
+#if GRUB_CPU_SIZEOF_LONG == 8
# define GRUB_ULONG_MAX 18446744073709551615UL
# define GRUB_LONG_MAX 9223372036854775807L
# define GRUB_LONG_MIN (-9223372036854775807L - 1)
[-- Attachment #2: Esto es una parte de mensaje firmado digitalmente --]
[-- Type: application/pgp-signature, Size: 835 bytes --]
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [RFC,PATCH] C99 format specifiers for fixed-length integer types
2009-07-22 19:10 [RFC,PATCH] C99 format specifiers for fixed-length integer types Javier Martín
@ 2009-07-23 1:08 ` Pavel Roskin
2009-07-23 2:03 ` Javier Martín
2009-07-23 18:54 ` Javier Martín
1 sibling, 1 reply; 11+ messages in thread
From: Pavel Roskin @ 2009-07-23 1:08 UTC (permalink / raw)
To: The development of GRUB 2
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
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [RFC,PATCH] C99 format specifiers for fixed-length integer types
2009-07-23 1:08 ` Pavel Roskin
@ 2009-07-23 2:03 ` Javier Martín
2009-07-23 2:31 ` Pavel Roskin
0 siblings, 1 reply; 11+ messages in thread
From: Javier Martín @ 2009-07-23 2:03 UTC (permalink / raw)
To: The development of GRUB 2
[-- Attachment #1: Type: text/plain, Size: 3045 bytes --]
El mié, 22-07-2009 a las 21:08 -0400, Pavel Roskin escribió:
> I doubt about "runtime weirdness". gcc is good at catching such
> problems at the compile time.
Yes, in naked expressions. However, once we start using casts the C
compiler tends to be quite silent about the whole nitty-gritty process.
For example, the following code generates no warnings (with -ansi
-pedantic -Wall -Wconversion):
unsigned short a = (unsigned short) 123456789;
uint16_t b = (uint16_t) UINT32_C(123456789);
That is, casts shut the compiler up (without the casts gcc just shows a
warning about the truncation being performed). The runtime value of both
a and b is 0xcd15 in my x86_64 Linux box.
> It would be a good idea to use standard modifiers if they were not so
> hard on the eye.
Well, I suggested GRUB_x names for the macros, where x is the C99
standard name, but we could use them directly as PRIx64 and the like.
About verbosity, though... Have you programmed in Java? Some appearances
of GRUB_PRId32 are nothing compared to System.out.println or
java.util.ArrayList<Integer>, yet I write the last two religiously.
> 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.
Hey, I'm the one proposing the patch. Surely you wouldn't expect me to
be unbiased ;)
Nevertheless, the issue you pointed happens because variadic arguments
are automatically promoted with the following rules [1][2] from the
ancient days of C:
- Integral types narrower than "int" -> int
- Floating point types narrower than "double" -> double
So, you could argue, we could do without at the very least PRI?8 (and,
here in grub, PRI?16 too). However, given how obscure this "feature" is
(see [1]), I'd go for keeping all of them: orthogonality means less
surprises for the future coders.
>
> 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.
Well, of course. This patch only adds the infrastructure, or else it
would be too invasive. Once it is in, a small number of people can start
checking most source files and replace the old specifiers when
appropriate.
>
> > -#if GRUB_CPU_SIZEOF_VOID_P == 8
> > +#if GRUB_CPU_SIZEOF_LONG == 8
>
> This belongs to a separate patch.
Oops, sorry. It seems I had a little mixup between the trunk files and
Bean's lib branch. I might post this change as a separate patch indeed.
[1] I would point at section 6.5.2.2.6 (function calls, automatic
argument promotions) of the C99 standard, but I really do not understand
that paragraph quite well.
[2] Here it says so with more certainty, but they seem to be class notes
http://www.eumus.edu.uy/eme/c/c-notes_summit/intermediate/sx11.html#sx11c
--
-- Lazy, Oblivious, Recurrent Disaster -- Habbit
[-- Attachment #2: Esto es una parte de mensaje firmado digitalmente --]
[-- Type: application/pgp-signature, Size: 835 bytes --]
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [RFC,PATCH] C99 format specifiers for fixed-length integer types
2009-07-23 2:03 ` Javier Martín
@ 2009-07-23 2:31 ` Pavel Roskin
0 siblings, 0 replies; 11+ messages in thread
From: Pavel Roskin @ 2009-07-23 2:31 UTC (permalink / raw)
To: The development of GRUB 2
On Thu, 2009-07-23 at 04:03 +0200, Javier Martín wrote:
> El mié, 22-07-2009 a las 21:08 -0400, Pavel Roskin escribió:
> > I doubt about "runtime weirdness". gcc is good at catching such
> > problems at the compile time.
> Yes, in naked expressions. However, once we start using casts the C
> compiler tends to be quite silent about the whole nitty-gritty process.
> For example, the following code generates no warnings (with -ansi
> -pedantic -Wall -Wconversion):
>
> unsigned short a = (unsigned short) 123456789;
> uint16_t b = (uint16_t) UINT32_C(123456789);
>
> That is, casts shut the compiler up (without the casts gcc just shows a
> warning about the truncation being performed). The runtime value of both
> a and b is 0xcd15 in my x86_64 Linux box.
That's totally expected. But it doesn't represent a full scenario when
something can go wrong when porting to another platform.
> > It would be a good idea to use standard modifiers if they were not so
> > hard on the eye.
> Well, I suggested GRUB_x names for the macros, where x is the C99
> standard name, but we could use them directly as PRIx64 and the like.
> About verbosity, though... Have you programmed in Java? Some appearances
> of GRUB_PRId32 are nothing compared to System.out.println or
> java.util.ArrayList<Integer>, yet I write the last two religiously.
We are not writing GRUB in Java. I guess Java also offers way to
simplify code and to check its correctness that we don't have in C.
> > 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.
> Hey, I'm the one proposing the patch. Surely you wouldn't expect me to
> be unbiased ;)
> Nevertheless, the issue you pointed happens because variadic arguments
> are automatically promoted with the following rules [1][2] from the
> ancient days of C:
> - Integral types narrower than "int" -> int
> - Floating point types narrower than "double" -> double
> So, you could argue, we could do without at the very least PRI?8 (and,
> here in grub, PRI?16 too). However, given how obscure this "feature" is
> (see [1]), I'd go for keeping all of them: orthogonality means less
> surprises for the future coders.
That's OK.
> > 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.
> Well, of course. This patch only adds the infrastructure, or else it
> would be too invasive. Once it is in, a small number of people can start
> checking most source files and replace the old specifiers when
> appropriate.
And that's what I don't want to see.
We sacrifice readability and go through massive code changes. We only
gain potential fixes for ports to hypothetical platforms.
In my opinion, it's not worth the trouble. I'm not going to spend any
more time on this. If you convince any maintainers, fine.
--
Regards,
Pavel Roskin
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [RFC,PATCH] C99 format specifiers for fixed-length integer types
2009-07-22 19:10 [RFC,PATCH] C99 format specifiers for fixed-length integer types Javier Martín
2009-07-23 1:08 ` Pavel Roskin
@ 2009-07-23 18:54 ` Javier Martín
2009-07-23 19:51 ` [RFC, PATCH] " Vladimir 'phcoder' Serbinenko
1 sibling, 1 reply; 11+ messages in thread
From: Javier Martín @ 2009-07-23 18:54 UTC (permalink / raw)
To: The development of GRUB 2
[-- Attachment #1.1: Type: text/plain, Size: 1106 bytes --]
Here is a new version which also incorporates the C99 integer constant
macros. To avoid excess verbosity, all macros have now names like Gx,
where x is the standard name. Thus, PRIx64, UINT64_C(a) --> GPRIx64,
GUINT64_C(a).
If the changes seem too invasive and/or the verbosity added is
unbearable, I'd suggest dropping the macros for 8 and 16 bits (and maybe
even 32), which are completely unnecessary under our current target
constraints and most conceivable ones. Thus, one would print an uint16_t
followed by an uint64_t like this (I know, it is a silly example):
uint16_t seg = 0x40;
uint64_t off = GUINT64_C(0x13);
grub_printf("int13 slot @ %04x:%016"GPRIx64", seg, off);
However, as I already said, orthogonality has a small but not negligible
importance, because it makes the world (seem) less weird. GUINT16_C and
GPRIx16 (for example) are not that long to type, particularly when 16bit
values are not that frequent, and it might help recognizability (aka
"what type is this variable?") for those without an IDE.
--
-- Lazy, Oblivious, Recurrent Disaster -- Habbit
[-- Attachment #1.2: c99intmacros.patch --]
[-- Type: text/x-patch, Size: 1802 bytes --]
Index: include/grub/types.h
===================================================================
--- include/grub/types.h (revisión: 2439)
+++ include/grub/types.h (copia de trabajo)
@@ -58,23 +58,63 @@
# endif
#endif
-/* Define various wide integers. */
+/* Define various wide integers and their format specifiers. */
typedef signed char grub_int8_t;
typedef short grub_int16_t;
typedef int grub_int32_t;
+#define GPRIi8 "i"
+#define GPRIi16 "i"
+#define GPRIi32 "i"
+#define GPRId8 "d"
+#define GPRId16 "d"
+#define GPRId32 "d"
+#define GINT8_C(x) (x)
+#define GINT16_C(x) (x)
+#define GINT32_C(x) (x)
#if GRUB_CPU_SIZEOF_VOID_P == 8
typedef long grub_int64_t;
+#define GPRIi64 "li"
+#define GPRId64 "ld"
+#define GINT64_C(x) (x ## L)
#else
typedef long long grub_int64_t;
+#define GPRIi64 "lli"
+#define GPRId64 "lld"
+#define GINT64_C(x) (x ## LL)
#endif
typedef unsigned char grub_uint8_t;
typedef unsigned short grub_uint16_t;
typedef unsigned grub_uint32_t;
+#define GPRIu8 "u"
+#define GPRIu16 "u"
+#define GPRIu32 "u"
+#define GPRIo8 "o"
+#define GPRIo16 "o"
+#define GPRIo32 "o"
+#define GPRIx8 "x"
+#define GPRIx16 "x"
+#define GPRIx32 "x"
+#define GPRIX8 "X"
+#define GPRIX16 "X"
+#define GPRIX32 "X"
+#define GUINT8_C(x) (x ## U)
+#define GUINT16_C(x) (x ## U)
+#define GUINT32_C(x) (x ## U)
#if GRUB_CPU_SIZEOF_VOID_P == 8
typedef unsigned long grub_uint64_t;
+#define GPRIu64 "lu"
+#define GPRIo64 "lo"
+#define GPRIx64 "lx"
+#define GPRIX64 "lX"
+#define GUINT64_C(x) (x ## UL)
#else
typedef unsigned long long grub_uint64_t;
+#define GPRIu64 "llu"
+#define GPRIo64 "llo"
+#define GPRIx64 "llx"
+#define GPRIX64 "llX"
+#define GUINT64_C(x) (x ## ULL)
#endif
/* Misc types. */
[-- Attachment #2: Esto es una parte de mensaje firmado digitalmente --]
[-- Type: application/pgp-signature, Size: 835 bytes --]
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [RFC, PATCH] C99 format specifiers for fixed-length integer types
2009-07-23 18:54 ` Javier Martín
@ 2009-07-23 19:51 ` Vladimir 'phcoder' Serbinenko
2009-07-23 20:16 ` Pavel Roskin
0 siblings, 1 reply; 11+ messages in thread
From: Vladimir 'phcoder' Serbinenko @ 2009-07-23 19:51 UTC (permalink / raw)
To: The development of GRUB 2
2009/7/23 Javier Martín <lordhabbit@gmail.com>:
> Here is a new version which also incorporates the C99 integer constant
> macros. To avoid excess verbosity, all macros have now names like Gx,
> where x is the standard name. Thus, PRIx64, UINT64_C(a) --> GPRIx64,
> GUINT64_C(a).
Please respect current convention of using GRUB_ as prefix for macros
Are the macros *_C really useful? Anyway it's to be discussed
separately. Could you not do increase previous patch bt make a new one
to separate what is already in discussion from new things.
@Pavel: do you have any further comments?
> --
> -- Lazy, Oblivious, Recurrent Disaster -- Habbit
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> http://lists.gnu.org/mailman/listinfo/grub-devel
>
>
--
Regards
Vladimir 'phcoder' Serbinenko
Personal git repository: http://repo.or.cz/w/grub2/phcoder.git
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [RFC, PATCH] C99 format specifiers for fixed-length integer types
2009-07-23 19:51 ` [RFC, PATCH] " Vladimir 'phcoder' Serbinenko
@ 2009-07-23 20:16 ` Pavel Roskin
2009-07-23 20:38 ` Vladimir 'phcoder' Serbinenko
0 siblings, 1 reply; 11+ messages in thread
From: Pavel Roskin @ 2009-07-23 20:16 UTC (permalink / raw)
To: The development of GRUB 2
On Thu, 2009-07-23 at 21:51 +0200, Vladimir 'phcoder' Serbinenko wrote:
> 2009/7/23 Javier Martín <lordhabbit@gmail.com>:
> > Here is a new version which also incorporates the C99 integer constant
> > macros. To avoid excess verbosity, all macros have now names like Gx,
> > where x is the standard name. Thus, PRIx64, UINT64_C(a) --> GPRIx64,
> > GUINT64_C(a).
> Please respect current convention of using GRUB_ as prefix for macros
> Are the macros *_C really useful? Anyway it's to be discussed
> separately. Could you not do increase previous patch bt make a new one
> to separate what is already in discussion from new things.
> @Pavel: do you have any further comments?
I believe it's not worth the trouble at this point.
There are many patches that I make and never send or never commit
because I'm not sure that the change is valuable enough. Every change
comes with a risk of breaking something.
For instance, I planned to remove "cli" in the bootloader, but decided
that it's not worth the risk. I wanted to remove support for module
unloading, but decided that there is a downside for users, and there are
better ways to reduce the size of core.img. I wanted to simplify awk
invocation, but decided not to pursue it at this time, as it would
conflict with other efforts to reorganize the build system, and the
benefits would be mostly theoretical.
I think a good developer should be able to drop changes that are not
exactly useful and move on. We have other issues.
Javier mentioned -Wconversion. It makes the compiler very noisy, but
some issues it found are real, such as the problem with ALIGN_UP. It's
more important than pushing the same patch.
One day we may find a nice solution to the issue of file formats. Maybe
a new C standard appears with new format specifications. Maybe gcc will
add some checks. But as it stands now, we won't benefit from it enough
to justify less readable code. Let's move on and work on the issues
where we can make the difference now.
--
Regards,
Pavel Roskin
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [RFC, PATCH] C99 format specifiers for fixed-length integer types
2009-07-23 20:16 ` Pavel Roskin
@ 2009-07-23 20:38 ` Vladimir 'phcoder' Serbinenko
2009-07-23 21:25 ` Javier Martín
0 siblings, 1 reply; 11+ messages in thread
From: Vladimir 'phcoder' Serbinenko @ 2009-07-23 20:38 UTC (permalink / raw)
To: The development of GRUB 2
On Thu, Jul 23, 2009 at 10:16 PM, Pavel Roskin<proski@gnu.org> wrote:
> On Thu, 2009-07-23 at 21:51 +0200, Vladimir 'phcoder' Serbinenko wrote:
>> 2009/7/23 Javier Martín <lordhabbit@gmail.com>:
>> > Here is a new version which also incorporates the C99 integer constant
>> > macros. To avoid excess verbosity, all macros have now names like Gx,
>> > where x is the standard name. Thus, PRIx64, UINT64_C(a) --> GPRIx64,
>> > GUINT64_C(a).
>> Please respect current convention of using GRUB_ as prefix for macros
>> Are the macros *_C really useful? Anyway it's to be discussed
>> separately. Could you not do increase previous patch bt make a new one
>> to separate what is already in discussion from new things.
>> @Pavel: do you have any further comments?
>
> I believe it's not worth the trouble at this point.
>
> There are many patches that I make and never send or never commit
> because I'm not sure that the change is valuable enough. Every change
> comes with a risk of breaking something.
I also agree that *_C macros are not very useful and make code less
readable however I want to let JAvier speak why he wants this in GRUB
>
> Javier mentioned -Wconversion. It makes the compiler very noisy, but
> some issues it found are real, such as the problem with ALIGN_UP. It's
> more important than pushing the same patch.
>
There is also -Wdeclaration-after-statement which reports style
problems. I have currently a branch named nomixed which fixes all
these parts but may introduce bugs
> One day we may find a nice solution to the issue of file formats. Maybe
> a new C standard appears with new format specifications. Maybe gcc will
> add some checks. But as it stands now, we won't benefit from it enough
> to justify less readable code. Let's move on and work on the issues
> where we can make the difference now.
>
Ok. Let's look at video subsystem? My framebuffer changes are
necessary to get graphics support on EFI-based platforms (including
GOP). I know it's big but it's mostly moving the code around and
benefit would be easy implementation of new graphical drivers. Then we
could merge Collin D Benett's patches as much as is already ready to
go in
> --
> Regards,
> Pavel Roskin
>
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> http://lists.gnu.org/mailman/listinfo/grub-devel
>
--
Regards
Vladimir 'phcoder' Serbinenko
Personal git repository: http://repo.or.cz/w/grub2/phcoder.git
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [RFC, PATCH] C99 format specifiers for fixed-length integer types
2009-07-23 20:38 ` Vladimir 'phcoder' Serbinenko
@ 2009-07-23 21:25 ` Javier Martín
2009-07-23 21:48 ` Pavel Roskin
0 siblings, 1 reply; 11+ messages in thread
From: Javier Martín @ 2009-07-23 21:25 UTC (permalink / raw)
To: The development of GRUB 2
[-- Attachment #1.1: Type: text/plain, Size: 2417 bytes --]
Well, I'll try to address you both at once... Let me say that seeing
more messages arrive with fresh arguments as I tried to build a coherent
response was distressing ;)
--
Pavel, you are right. I recognize I've gone overboard trying to be the
paladin of standards, and thus I'd like to withdraw most of the patch.
The only part that is useful and might someday become necessary is that
dealing with 64-bit integers. In particular, if and when mingw64 support
is added, for the reasons stated before.
You could argue that we could just use "long long" and its format
specifiers for all 64-bit integers everywhere, since it has that length
in all supported platforms. However, we have no guarantees this will
continue to be so in the future even for our currently supported
platforms, and GCC 5.0 might as well have a 128-bit "long long" (MIPS
already has an uint128_t), so making printf extract 16 bytes with "%ll"
and give it only 8 with an uint64_t argument could be the kind of fun
that takes a programmer hours to debug.
With the reduced version of the patch I'm putting forward, such a
(hypothetical, indeed) change will only impact types.h, while otherwise
many source files will need to be modified in a hunt for "%ll"s and
their variations. We can consider "lower" types safe since the
autopromotion rules will keep the compiler happy even if int becomes
64-bit.
--
Vladimir, I've followed your advice and restored the names for the
remaining macros in this new patch. I'm open to any compromise between
the GRUB_ convention and the need for less verbosity, given that 64-bit
numbers are used widely throughout the code base. Conventions are just
ways to make the life of us coders easier, but, without showing
disrespect for them, they are not dogma. We must be ready to shove them
apart if they get in the way.
Respect to the utility of the [U]INTn_C(x) macros, they append the
required modifiers to make sure the compiler interprets x as an
[u]intn_t constant. For example, UINT64_C(1) -> 1uL in my amd64 Ubuntu
box, but 1uLL in my WinXP Pro x64 box with mingw64 gcc. I agree that
adding them goes outside the scope of the announced "format specifiers",
but one without the other is pretty much useless in most real use cases,
as I noticed when trying to use my initial patch in real GRUB code.
--
-- Lazy, Oblivious, Recurrent Disaster -- Habbit
[-- Attachment #1.2: int64macros.patch --]
[-- Type: text/x-patch, Size: 1080 bytes --]
Index: include/grub/types.h
===================================================================
--- include/grub/types.h (revision 2440)
+++ include/grub/types.h (working copy)
@@ -64,8 +64,14 @@
typedef int grub_int32_t;
#if GRUB_CPU_SIZEOF_VOID_P == 8
typedef long grub_int64_t;
+#define GRUB_PRIi64 "li"
+#define GRUB_PRId64 "ld"
+#define GRUB_INT64_C(x) (x ## L)
#else
typedef long long grub_int64_t;
+#define GRUB_PRIi64 "lli"
+#define GRUB_PRId64 "lld"
+#define GRUB_INT64_C(x) (x ## LL)
#endif
typedef unsigned char grub_uint8_t;
@@ -73,8 +79,18 @@
typedef unsigned grub_uint32_t;
#if GRUB_CPU_SIZEOF_VOID_P == 8
typedef unsigned long grub_uint64_t;
+#define GRUB_PRIu64 "lu"
+#define GRUB_PRIo64 "lo"
+#define GRUB_PRIx64 "lx"
+#define GRUB_PRIX64 "lX"
+#define GRUB_UINT64_C(x) (x ## UL)
#else
typedef unsigned long long grub_uint64_t;
+#define GRUB_PRIu64 "llu"
+#define GRUB_PRIo64 "llo"
+#define GRUB_PRIx64 "llx"
+#define GRUB_PRIX64 "llX"
+#define GRUB_UINT64_C(x) (x ## ULL)
#endif
/* Misc types. */
[-- Attachment #2: Esto es una parte de mensaje firmado digitalmente --]
[-- Type: application/pgp-signature, Size: 835 bytes --]
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [RFC, PATCH] C99 format specifiers for fixed-length integer types
2009-07-23 21:25 ` Javier Martín
@ 2009-07-23 21:48 ` Pavel Roskin
2009-07-25 15:14 ` Javier Martín
0 siblings, 1 reply; 11+ messages in thread
From: Pavel Roskin @ 2009-07-23 21:48 UTC (permalink / raw)
To: The development of GRUB 2
On Thu, 2009-07-23 at 23:25 +0200, Javier Martín wrote:
> With the reduced version of the patch I'm putting forward, such a
> (hypothetical, indeed) change will only impact types.h, while otherwise
> many source files will need to be modified in a hunt for "%ll"s and
> their variations. We can consider "lower" types safe since the
> autopromotion rules will keep the compiler happy even if int becomes
> 64-bit.
OK, I'm fine with this change. It would only address the problem we
have now (long vs .long long for 64-bit types) without trying to
anticipate what other platforms we may support.
--
Regards,
Pavel Roskin
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [RFC, PATCH] C99 format specifiers for fixed-length integer types
2009-07-23 21:48 ` Pavel Roskin
@ 2009-07-25 15:14 ` Javier Martín
0 siblings, 0 replies; 11+ messages in thread
From: Javier Martín @ 2009-07-25 15:14 UTC (permalink / raw)
To: The development of GRUB 2
[-- Attachment #1: Type: text/plain, Size: 1111 bytes --]
El jue, 23-07-2009 a las 17:48 -0400, Pavel Roskin escribió:
> On Thu, 2009-07-23 at 23:25 +0200, Javier Martín wrote:
>
> > With the reduced version of the patch I'm putting forward, such a
> > (hypothetical, indeed) change will only impact types.h, while otherwise
> > many source files will need to be modified in a hunt for "%ll"s and
> > their variations. We can consider "lower" types safe since the
> > autopromotion rules will keep the compiler happy even if int becomes
> > 64-bit.
>
> OK, I'm fine with this change. It would only address the problem we
> have now (long vs .long long for 64-bit types) without trying to
> anticipate what other platforms we may support.
>
OK then... what about this for a ChangeLog entry?
2009-07-25 Javier Martín <lordhabbit@gmail.com>
* include/grub/types.h (GRUB_PRIi64): new macro
(GRUB_PRId64): likewise
(GRUB_PRIu64): likewise
(GRUB_PRIo64): likewise
(GRUB_PRIx64): likewise
(GRUB_PRIX64): likewise
(GRUB_INT64_C): likewise
(GRUB_UINT64_C): likewise
--
-- Lazy, Oblivious, Recurrent Disaster -- Habbit
[-- Attachment #2: Esto es una parte de mensaje firmado digitalmente --]
[-- Type: application/pgp-signature, Size: 835 bytes --]
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2009-07-25 15:14 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2009-07-22 19:10 [RFC,PATCH] C99 format specifiers for fixed-length integer types Javier Martín
2009-07-23 1:08 ` Pavel Roskin
2009-07-23 2:03 ` Javier Martín
2009-07-23 2:31 ` Pavel Roskin
2009-07-23 18:54 ` Javier Martín
2009-07-23 19:51 ` [RFC, PATCH] " Vladimir 'phcoder' Serbinenko
2009-07-23 20:16 ` Pavel Roskin
2009-07-23 20:38 ` Vladimir 'phcoder' Serbinenko
2009-07-23 21:25 ` Javier Martín
2009-07-23 21:48 ` Pavel Roskin
2009-07-25 15:14 ` Javier Martín
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.