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

Kaushik Phatak
2012-10-09T13:20:34Z

Hi Joel,

> I think that there is a log of stuff in cr16-tdep that is actually
> linux-specific, and that you should move to cr16-linux-tdep.c
I have moved register initializations into the cr16-linux-tdep.c. This
was also recommended by Yao.

> Take a look at how sparc is organized, or maybe amd64.
The other targets like amd64 have plenty on stuff in this file, however it is
unrelated to this port. I feel the updated cr16-linux-tdep.c file should allow
it to build on top of the bareboard one.

> Needs a comment. For global variables and constants
Added comments at appropriate location in the code.

> have an empty line between the documentation/comment and the function.
Added empty lines between comment and functions

>> +extern ins currInsn;
> This looks strange. Why extern, and where would they come from?
I have added comments for this and removed the unnecessary externs here.

> Don't these functions have declarations in opcode? This is really ugly
These prototypes were added recently and are part of a bfd patch. I forgot
to remove these in the last version.

> +extern const gdb_byte breakpoint_linux[];
I have removed this as it is not used in the later versions of the patch.

> The last line isn't aligned properly.
> So this paragraph needs to be reformatted.
> requires that 2 spaces be used after periods.
> Please also move the closing "*/" to the end of the last line
> The '{' needs to be on the next line.
> Trailing space at the end of the second line
> move the closing */ to the second line as well.
> And empty line between comment
> moved the last comment to just above the field

I have made another pass over the code and have taken care of the above
mentioned points and other formatting nits which I could find. Thanks.

I have not attached my updated patch here. I am working on Yao's comments
and will attach it in that reply.

Regards,
Kaushik