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

[Tristan Gingold]

Hi Paulo,

do you agree on this new version of the patch ?

Tristan.

On Jun 18, 2008, at 9:59 AM, Tristan Gingold wrote:

> 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>_______________________________________________
> AVR-GCC-list mailing list
> address@hidden
> http://lists.nongnu.org/mailman/listinfo/avr-gcc-list