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

[Bill Gatliff <bgat@billgatliff.com>]

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@ca.kontron.com> 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@billgatliff.com