Re: [avr-gcc-list] Patch avrtest: add support for avr6

[Tristan Gingold]

On Jun 18, 2008, at 8:14 AM, Paulo Marques wrote:

Hi Paulo,

thank you for the quick review.

> I don't know if it makes sense to use bit fields here, because  
> there might be a performance penalty in accessing them and  
> "pc_3bytes" is accessed a few times while executing avr code.

Changed to unsigned char.

> > + // List of supported archs with their features.
> > + const struct arch_desc arch_descs[] =
> > + {
> > +       { "avr51", 0, 0},
> > +       { "avr6", 1, 1},
> > +       { NULL, 0, 0}
> > + };
>
> It would probably bew easier to read these structures if they were  
> written like:
>
> const struct arch_desc arch_descs[] = {
>   {
>     .name = "avr51",
>     .pc_3_bytes = 0,
>     .has_eind = 0,
>   },
>   {
>     .name = "avr6",
>     .pc_3_bytes = 1,
>     .has_eind = 1,
>   },
>   { NULL, 0, 0}
> };
>
> I know its more verbose, but if we keep adding flags it will become  
> impossible to understand what { "avr51", 0, 0, 1, 0, 1, 1, 0 }  
> means ;)

I agree.

> You forgot to update usage() to reflect the new options, no?

Oops, you're right.

> I don't think we need to restrict the program name to be the last  
> argument.

Ok.

> >                 }
> >         }
> >   !     // setup default values
> > ! 	flash_addr_mask = (arch->pc_3bytes ? MAX_FLASH_SIZE : (1 <<  
> > 16)) - 1;
>
> flash_addr_mask is in bytes, so to keep the previous default 128kB  
> this should be "(1 << 17) - 1", no?

Oops, you're right.  Thank you for catching this!

> Anyway, overall this seems like a nice simple structure to add more  
> architectures and features in the future.

Attached is the updated patch.

Thanks,
Tristan.

avr6.diff
Description: Binary data