Re: [PATCH v3] sim: bfin: new port

[Joel Brobecker <brobecker@adacore.com>]

Please, sending compressed files makes it harder to look at the file.
I think you were right to do it for a file that's 4MB, but in that case,
using  something a little more usual like gz, zip or bzip2 would have
made things easier... I had to install the xzutil on my laptop and it
wasn't without its little scary moment...

> changes since v3:
>  - stubbed all the bootroms (i'll follow up)
>  - add simple jtag/watchpoint device simulation

I'm not a sim expert, so I can only provide cosmetic review.
However, I did notice a couple of important things: The copyright
headers should mention 2011, and the license should be GPL version 3.

Question: Why do you have .h files whose name starts with an underscore?
For instance: sim/bfin/_proc_list.h...

General comment: We should really strive to follow the same coding
standards in sim/ as we do in gdb/. In particular, it would be nice
to have more comments, and to follow the maximum line length as
possible.

For instance:

> typedef enum
> {
>   c_0, c_1, c_4, c_2, c_uimm2, c_uimm3, c_imm3, c_pcrel4,
>   c_imm4, c_uimm4s4, c_uimm4s4d, c_uimm4, c_uimm4s2, c_negimm5s4, c_imm5, c_imm5d, c_uimm5, c_imm6,
>   c_imm7, c_imm7d, c_imm8, c_uimm8, c_pcrel8, c_uimm8s4, c_pcrel8s4, c_lppcrel10, c_pcrel10,

For the comments - I think that's a lot of code now, and I wont' block
the commit for something like that.  But, to me, code documentation is
an integral part of software quality... For instance, the following
function has a relatively cryptic name, particularly for someone who
is not familiar with the bfin architecture:

> +static const char *
> +fmtconst_str (const_forms_t cf, bs32 x, bu32 pc)

Remove the '*' at the beginning of each line in multi-line comments:

>  /* If an operation would otherwise cause a positive value to overflow
>   * and become negative, instead, saturation limits the result to the
>   * maximum positive value for the size register being used.

Binary operators should be at the start of the line rather than
at the end:

>  if ((gs < 2) ||                              /* Dregs/Pregs as source  */
>      (gd < 2) ||                              /* Dregs/Pregs as dest    */
>      (gs == 4 && src < 4) ||                  /* Accumulators as source */
>      (gd == 4 && dst < 4 && (gs < 4)) ||      /* Accumulators as dest   */

I am pretty sure you already know all this; it's just that it's so
much code that it's hard to always be complient, particularly if
the code started in something like 2005.

I have no other comment. I think you know what you're doing in
the simulator area, so I'm inclined to trust your judgment on
the rest of the code (if you could just remember for future
contributions that comments are important). If Pedro has no further
comments, the meat of the patch is OK with me.

-- 
Joel (burp - 100K lines of patch later)