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

[Mike Frysinger <vapier@gentoo.org>]

[-- Attachment #1: Type: Text/Plain, Size: 2744 bytes --]

On Monday, February 21, 2011 04:54:36 Joel Brobecker wrote:
> 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...

sorry, i'm so used to xz i expect everyone to have it ;)

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

all the files do say 2011 that i can see ... were there ones missing that you 
saw ?

much of the sim project seems to still be GPL v2, so i didnt want to try and 
figure out what's going on.  if this code should be GPL v3, that's fine, i can 
relicense it.

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

it's meant to convey that the header isnt meant to be generally included by 
random files.  they can only be included in specific instances since they're 
simple lists which need certain preprocessor directives in place before they 
can be included.  i'm trying to avoid bit rot related to copying & pasting the 
same list of numbers over and over in multiple places.

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

it is supposed to be the GNU coding style.  if it isnt, then it's just an 
oversight.  i'll go through and try to fix up missing pieces including the 
ones you highlighted.

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

this section is simply copy & pasted from the existing disassembler.  at some 
point i'd like to unify it.
-mike

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 836 bytes --]