Re: [RFA 3/5] New port: CR16: gdb port

[Pedro Alves <palves at redhat dot com>]

On 01/17/2013 08:59 AM, Joel Brobecker wrote:

>> +      /* Read 6 bytes, max 48 bit opcode.  */
>> +      target_read_memory (pc, buf, 6);
>> +      cr16_words[0] = buf[1] << 8 | buf[0];
>> +      cr16_words[1] = buf[3] << 8 | buf[2];
>> +      cr16_words[2] = buf[5] << 8 | buf[4];
>> +      cr16_allWords = (((ULONGLONG) cr16_words[0] << 32)
>> +		      + ((ULONGLONG) cr16_words[1] << 16)
>> +		      + cr16_words[2]);
>> +
>> +      /* Find a matching opcode in table.
>> +         Nonzero means instruction has a match.  */
>> +      is_decoded = cr16_match_opcode ();
>> +      cr16_make_instruction ();
>> +      length = cr16_currInsn.size;
> 
> It hurts every time I read this code... Nothing you can do short
> of improving opcode, but this is really awful :-(.

This seems pretty isolated.  How about exporting a function that
hides these opcodes details?  I don't even pretend to understand
what the code is trying to do, and it'd be an opportunity to
comment it in the function description.  :-)

     /* Take BUF, do something with it, and write length
        to LENGTH.  Blah, blah.  */
     cr16_do_something (buf, *length, ...);




> static const char *const reg_names[] =
> +{
...
> +  "r0r1_orig",

This too looks like a ptrace detail escaping all the
way to the user, similar to the gdbserver issues.
Any reason not to split those up?  I think it'd be nicer.

...
> +};

Joel Brobecker wrote:
> The rest still looks pretty good to me :)

I agree.  :-)

-- 
Pedro Alves