grub-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [RFC] Framebuffer rotation patch


From: Michal Suchanek
Subject: Re: [RFC] Framebuffer rotation patch
Date: Tue, 16 Feb 2010 17:08:28 +0100

2010/2/11 Vladimir 'φ-coder/phcoder' Serbinenko <address@hidden>:
> Michal Suchanek wrote:
>> Hello
>>
>> Sending a preliminary framebuffer rotation patch.
>>
>> You can use videotest to see 4 tiles rotated from the same bitmap data.
>>
>>
> +char leaf_data[] = { 0x00, 0x0f, 0xe0, 0x00,
> +                     0x00, 0x7f, 0xfc, 0x00,
> +                     0x01, 0xff, 0xff, 0x00,
> +                     0x03, 0xff, 0xff, 0x80,
> This is a blob. Could it be generated automatically at build time?
> +
> +  sans = grub_font_get ("Helvetica Bold 14");
> Please use free font rather than Helvetica
> Could you split addition of videotests from the rest of the patch?
> -    unsigned int x;
> -    unsigned int y;
> -    unsigned int width;
> -    unsigned int height;
> +    int x;
> +    int y;
> +    int width;
> +    int height;
> Why do you need negative values?
> +/* Supported operations are simple and easy to understand.
> + * MIRROR  |  swap image across (around) the vertical axis
> + * FLIP    -  swap image across the horizontal axis - upside down
> + * SWAP    /  swap image across the x=y axis - swap the x and y coordinates
> It's just a D_8 group. Could you add a comment to functions what they do
> in group theoretical sense? It would make the code easier to follow and
> compute transformations for mathematicians. Perhaps another
> representation of D_8 would result in more efficient code?
> +static inline void grub_swap_int(int *a, int *b) { int tmp = *a; *a =
> *b; *b = tmp; }
> +static inline void grub_swap_unsigned(unsigned *a, unsigned *b) {
> unsigned tmp = *a; *a = *b; *b = tmp; }
> +
> With typeof macro this can be made type-neutral avoiding potential mistakes.
> +static inline long
> +grub_min (long x, long y)
> +{
> +  if (x > y)
> +    return y;
> +  else
> +    return x;
> +}
> +

I don't see how typeof would be used. As I understand the docs it can
only set types relative to something what is already defined (and in
some cases actually dereference/call it) and there is nothing defined
at the point these functions are declared to copy the type from.

Still the grub_min being long is somewhat suspicious. iirc I just
reused an existing function or copied grub_max.

If declaring it as long can cause problems then its utility as a
general purpose function is dubious.

Thanks

Michal




reply via email to

[Prev in Thread] Current Thread [Next in Thread]