Re: [PATCH] Check for the appropriate condition in types.h

[Javier Martín]

El jue, 23-07-2009 a las 10:37 -0400, Pavel Roskin escribió:
> On Thu, 2009-07-23 at 13:10 +0200, Javier Martín wrote:
> > Currently <grub/types.h> verifies that sizeof(void*)==sizeof(long) and
> > then proceeds to define most fixed-length types based on sizeof(void*).
> > While simplification seems a good idea on paper, it is always good
> > practice to check specifically for what we want to know. In particular,
> > the assumption that long can hold a pointer will go down when (if)
> > mingw64 support is merged, because Win64 follows the LLP64 model instead
> > of the LP64 *nix model.
> > 
> > Given that currently both values are, by precondition, the same, this
> > change is guaranteed not to create any problems, while it might avoid
> > them in the future.
> 
> The patch is good.  Please include the ChangeLog entry.
> 
Thanks. I have though of an additional change: if it seems acceptable,
apply the "new" version of the patch that I attach with this message. If
it is not, just use the old version in the original post and I'll send
the new change as another patch.

The additional change is the refactoring of the UINT_TO_PTR macro. Given
that we have a grub_addr_t type fulfilling a role similar to the
standard uintptr_t (an integral type "long enough to hold a pointer and
back"), there is no need for the conditional definition of UINT_TO_PTR
as either casting to a 32 or 64-bit type: grub_addr_t is defined exactly
like that.

Furthermore, I have added a PTR_TO_UINT macro as a means to perform
safer* "hard" pointer arithmetic that frequently comes up in low-level
code without having to think of the types to cast to and back. This way,
this random code: (in acpi.c)
        target = (grub_uint8_t *) ((((long) target - 1) | 0xf) + 1);
Would become:
        target = UINT_TO_PTR(((PTR_TO_UINT(target) - 1) | 0xf) + 1);
Which is safe* as long as grub_addr_t is properly defined. Thus, this
new macro will allow coders to gather scattered typing decisions and
centralize them to types.h. The process would be just: PTR_TO_UINT ->
perform weird arithmetic -> UINT_TO_PTR.


* safer in the sense of not risking any truncations or "undefined
behavior" if the addr_t types change, of course, not for the actual
arithmetic being performed


Regarding the ChangeLog entry, I always have problems with them. What
about this one?:
2009-07-23 Javier Martin <address@hidden>
        * include/grub/types.h (GRUB_CPU_SIZEOF_VOID_P): substitute for
          GRUB_CPU_SIZEOF_LONG where appropriate
          (UINT_TO_PTR): move outside wordsize conditionals
          (PTR_TO_UINT): new macro

-- 
-- Lazy, Oblivious, Recurrent Disaster -- Habbit

checkforlong.1.patch
Description: Text Data

signature.asc
Description: Esto es una parte de mensaje firmado digitalmente