Re: Patch of a new driver for kernel 2.4.x that need review

[Bill Gatliff]

Willy:

Willy Tarreau wrote:

> Hi,
>
> On Wed, Jun 22, 2005 at 10:43:40PM +0300, Pekka Enberg wrote:
>  
>
> > On 6/22/05, Bouchard, Sebastien <Sebastien.Bouchard@xxxxxxxxxxxxxx> wrote:
> >    
> >
> > > +#define TLCLK_REG7 TLCLK_BASE+7
> > >      
> > Please use enums instead.
> >    
>
> I dont agree with you here : enums are good to simply specify an ordering.
> But they must not be used to specify static mapping. Eg: if REG4 *must* be
> equal to BASE+4, you should not use enums, otherwise it will render the
> code unreadable. I personnaly don't want to count the position of REG7 in
> the enum to discover that it's at BASE+7.
>  

What Sebastien is after is something like this:

	enum tclk_regid {TCLK_BASE=0xa80, TCLK_REG0=TCLK_BASE, TCLK_REG1=TCLK_BASE+1...};
	enum tclk_regid tclk;

And then later on, if you ask gdb with the value of tclk is, it can tell you "TCLK_REG1", instead of just 0xa801.  You can also assign values to tclk from within gdb using the enumerations, rather than magic numbers.

If you insist on using #defines, then you need to do them like this at the very least:

	#define TCLK_REG7 (TCLK_BASE+7)

... so as to prevent operator precedence problems later on.  I.e. what happens here:

	tclk = TCLK_REG7 / 2;

Not implying that the above is a realistic example, I'm just pointing out a potential gotcha that is easily avoided...



b.g.

--
Bill Gatliff
"A DTI spokesman said Wicks would use his debut announcement to
'reaffirm the government's commitment to developing wind' to tackle
the threat of climate change." -- The Observer, May 22, 2005
bgat@xxxxxxxxxxxxxxx

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/