Re: [Qemu-devel] [PATCH v2 11/13] tcx: remove primitives for non-32-bit surfaces

[Mark Cave-Ayland]

On 25/04/17 15:54, Peter Maydell wrote:

> On 21 April 2017 at 09:28, Mark Cave-Ayland
> <address@hidden> wrote:
>> As all surfaces in QEMU are now either shared or 32-bit ARGB regardless of
>> the guest depth, remove all non-32-bit primitives from tcx_update_display()
>> and consequence their implementation which are no longer required.
>>
>> Signed-off-by: Mark Cave-Ayland <address@hidden>
>> Reviewed-by: Gerd Hoffmann <address@hidden>
>> ---
>>  hw/display/tcx.c |  126 
>> ++++--------------------------------------------------
>>  1 file changed, 8 insertions(+), 118 deletions(-)
>>
>> diff --git a/hw/display/tcx.c b/hw/display/tcx.c
>> index 0dd007e..6da6dfb 100644
>> --- a/hw/display/tcx.c
>> +++ b/hw/display/tcx.c
>> @@ -140,25 +140,12 @@ static void update_palette_entries(TCXState *s, int 
>> start, int end)
>>      int i;
>>
>>      for (i = start; i < end; i++) {
>> -        switch (surface_bits_per_pixel(surface)) {
>> -        default:
>> -        case 8:
>> -            s->palette[i] = rgb_to_pixel8(s->r[i], s->g[i], s->b[i]);
>> -            break;
>> -        case 15:
>> -            s->palette[i] = rgb_to_pixel15(s->r[i], s->g[i], s->b[i]);
>> -            break;
>> -        case 16:
>> -            s->palette[i] = rgb_to_pixel16(s->r[i], s->g[i], s->b[i]);
>> -            break;
>> -        case 32:
>> -            if (is_surface_bgr(surface)) {
>> -                s->palette[i] = rgb_to_pixel32bgr(s->r[i], s->g[i], 
>> s->b[i]);
>> -            } else {
>> -                s->palette[i] = rgb_to_pixel32(s->r[i], s->g[i], s->b[i]);
>> -            }
>> -            break;
>> +        if (is_surface_bgr(surface)) {
>> +            s->palette[i] = rgb_to_pixel32bgr(s->r[i], s->g[i], s->b[i]);
>> +        } else {
>> +            s->palette[i] = rgb_to_pixel32(s->r[i], s->g[i], s->b[i]);
>>          }
>> +        break;
>>      }
> 
> Something's gone wrong here. Coverity points out that we now
> have a for() loop that only executes once, because it ends
> with a "break" statement. Looks like the 'break' from inside
> the 'case 32:' was retained when the switch was deleted,
> and it should just be deleted?

Ooops, yes indeed it looks like I fat-fingered the delete. I'll send a
patch to remove this.


ATB,

Mark.