2010/2/11 Vladimir 'φ-coder/phcoder' Serbinenko <phco...@gmail.com>:
> 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
_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/grub-devel
