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

[Michael Rolnik]

1. no more custings.
2. yes, it is generated code. I would like to commit the generator. however
it is not very ripe yet. and it has a dependency on yaml-cpp and xsltproc.
3. the generator does not assume that same bits are extracted. it will be
fixed later.
4. 0 is no longer returned.

On Sun, Jun 5, 2016 at 2:00 AM, Richard Henderson <address@hidden> wrote:

> 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~
>
>


-- 
Best Regards,
Michael Rolnik