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

Pedro Alves
2013-01-18T18:25:33Z

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