Re: [PATCH v4] lib/vsprintf: Add support for generic FOURCCs by extending %p4cc

[andriy.shevchenko@xxxxxxxxxxxxxxx]

On Thu, Feb 27, 2025 at 06:58:24PM +0000, Aditya Garg wrote:
> > On 27 Feb 2025, at 10:40 PM, Aditya Garg <gargaditya08@xxxxxxxx> wrote:
> >>> On 27 Feb 2025, at 8:13 PM, andriy.shevchenko@xxxxxxxxxxxxxxx wrote:
> >>> On Thu, Feb 27, 2025 at 06:30:48AM +0000, Aditya Garg wrote:

...

> >>> +    switch (fmt[2]) {
> >>> +    case 'h':
> >>> +        val = orig;
> >>> +        break;
> >>> +    case 'r':
> >>> +        orig = swab32(orig);
> >>> +        val = orig;
> >>> +        break;
> >>> +    case 'l':
> >>> +        orig = (__force u32)cpu_to_le32(orig);
> >>> +        val = orig;
> >>> +        break;
> >>> +    case 'b':
> >>> +        orig = (__force u32)cpu_to_be32(orig);
> >>> +        val = orig;
> >>> +        break;
> >>> +    case 'c':
> >>> +        /* Pixel formats are printed LSB-first */
> >>> +        val = swab32(orig & ~BIT(31));
> >>> +        pixel_fmt = true;
> >>> +        break;
> >>> +    default:
> >>> +        return error_string(buf, end, "(%p4?)", spec);
> >>> +    }
> >> 
> >> Actually you can replace all these orig copies by introducing a new boolean, pixel_be.
> >> 
> >> Will become
> >> 
> >>   switch (fmt[2]) {
> >>   case 'h':
> >>       val = orig;
> >>       break;
> >>   case 'r': // or 'n' ?
> >>       val = swab32(orig);
> >>       break;
> >>   case 'l':
> >>       val = (__force u32)cpu_to_le32(orig);
> >>       break;
> >>   case 'b':
> >>       val = (__force u32)cpu_to_be32(orig);
> >>       break;
> >>   case 'c':
> >>       pixel_fmt = true;
> >>       pixel_be = orig & BIT(31);
> >>       /* Pixel formats are printed LSB-first */
> >>       val = swab32(orig & ~BIT(31));
> >>       break;
> >>   default:
> >>       return error_string(buf, end, "(%p4?)", spec);
> >>   }
> >> 
> >> And with this the existence of 'val' now becomes doubtful, we may reuse 'orig',
> >> just name it 'val' everywhere, no?
> > 
> > In case c, val != orig, in rest it is. We can just use pixel_fmt to check this condition, but places where we use orig, and not val will need an if statement or something similar. Tbh, it's an unecessary complication. You might want to see this part of the code:
> 
> More easier IMO can be:
> 
> val = pixel_fmt ? swab32(orig & ~BIT(31)) : orig;
> 
> At the end of the table

Also will work for me. Just think about it, i.e. how to make code shorter and
still readable.

-- 
With Best Regards,
Andy Shevchenko