Re: [Qemu-devel] [PATCH 07/10] target-avr: adding instruction decoder

[Richard Henderson]

On 06/02/2016 01:06 PM, Michael Rolnik wrote:
> +uint32_t    avr_decode(uint32_t pc, uint32_t *length, uint32_t code, 
> translate_function_t *translate)
> +{
> +    uint32_t    opcode  = extract32(code, 0, 16);
> +    switch (opcode & 0x0000d000) {
> +        case    0x00000000: {
> +            uint32_t    opcode  = extract32(code, 0, 16);
> +            switch (opcode & 0x00002c00) {
> +                case    0x00000000: {
> +                    uint32_t    opcode  = extract32(code, 0, 16);

Why do you keep extracting the same value into variables that shadow each other?

I can only assume that this is machine-generated code.  Is there any reason 
that you want to commit the quite unreadable generated code instead of the 
original input and generator?

> +                            *translate = 
> (translate_function_t)&avr_translate_NOP;

The casts are, in this and other cases, unnecessary, and could easily mask a 
real problem.

> +    return  0;

Since you only ever return 0, what's the point of the return value?


r~