Re: [PATCH v4 1/6] drm/format-helper: Add drm_fb_xrgb8888_to_gray8_line()

[Geert Uytterhoeven]

Hi Jani,

On Fri, Feb 11, 2022 at 1:06 PM Jani Nikula <jani.nikula@xxxxxxxxxxxxxxx> wrote:
> On Fri, 11 Feb 2022, Thomas Zimmermann <tzimmermann@xxxxxxx> wrote:
> > Am 11.02.22 um 12:12 schrieb Andy Shevchenko:
> >> On Fri, Feb 11, 2022 at 11:40:13AM +0100, Javier Martinez Canillas wrote:
> >>> On 2/11/22 11:28, Andy Shevchenko wrote:
> >>>> On Fri, Feb 11, 2022 at 10:19:22AM +0100, Javier Martinez Canillas wrote:
> >>
> >> ...
> >>
> >>>>> +static void drm_fb_xrgb8888_to_gray8_line(u8 *dst, const u32 *src, unsigned int pixels)
> >>>>> +{
> >>>>> + unsigned int x;
> >>>>> +
> >>>>> + for (x = 0; x < pixels; x++) {
> >>>>> +         u8 r = (*src & 0x00ff0000) >> 16;
> >>>>> +         u8 g = (*src & 0x0000ff00) >> 8;
> >>>>> +         u8 b =  *src & 0x000000ff;
> >>>>> +
> >>>>> +         /* ITU BT.601: Y = 0.299 R + 0.587 G + 0.114 B */
> >>>>> +         *dst++ = (3 * r + 6 * g + b) / 10;
> >>>>> +         src++;
> >>>>> + }
> >>>>
> >>>> Can be done as
> >>>>
> >>>>    while (pixels--) {
> >>>>            ...
> >>>>    }
> >>>>
> >>>> or
> >>>>
> >>>>    do {
> >>>>            ...
> >>>>    } while (--pixels);
> >>>>
> >>>
> >>> I don't see why a while loop would be an improvement here TBH.
> >>
> >> Less letters to parse when reading the code.
> >
> > It's a simple refactoring of code that has worked well so far. Let's
> > leave it as-is for now.
>
> IMO *always* prefer a for loop over while or do-while.

(guess what ;-) I tend to disagree.

> The for (i = 0; i < N; i++) is such a strong paradigm in C. You
> instantly know how many times you're going to loop, at a glance. Not so
> with with the alternatives, which should be used sparingly.

In this case it's fairly obvious, and you get rid of the extra variable x.
Less code, less variables, what can go wrong? ;-)

> And yes, the do-while suggested above is buggy, and you actually need to
> stop and think to see why.

Yes, that one is wrong.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds