[LinuxBIOS] [RFC] Macros vs. functions for better readability

[Carl-Daniel Hailfinger]

Hi,

it seems we sometimes have lots of repetitive code sequences which could
be simplified by use of macros or functions. For a really shining
example see Torsten Duwe's recent patch titled "beautify m57sli mptable.c".
Cut-and-paste errors can happen and eliminating repetitive code reduces
the risk of such errors. Besides that, having code with shorter lines
through factoring out open-coded functions makes understanding the code
easier.

How do we factor out such code? I prefer macros for simple function
wrappers and functions for multiline sequences.

Comments?

Regards,
Carl-Daniel

[ron minnich]

On Jan 6, 2008 9:32 AM, Carl-Daniel Hailfinger
 wrote:
> How do we factor out such code? I prefer macros for simple function
> wrappers and functions for multiline sequences.

I spend a lot of time with Plan 9 code nowadays. These are the Bell
Labs guys who invented it all. I am (re)-learning a lot about style.
The Plan 9 code is so clean and sparse, it' s a real joy to read.

I think macros are OK for bits like convenience in struct deref and
such things. Spin locks and stuff like outb maybe make sense too.

I think the value of putting a one line function into a macro is
over-rated. It can badly bloat the code, in unexpected ways, And, look
at linux nowadays: lots of code living in .h files, making it a real
headache to figure out where things get done.

So I vote for your code cleanup idea, but I'd like to see it weighed
in favor of functions, unless a macro is somehow absolutely essential.

Thanks, it is a great idea, don't let me sound discouraging! The only
question is, should we be really trying to apply this lesson to V3?

ron

[Juergen Beisert]

On Sunday 06 January 2008 18:32, Carl-Daniel Hailfinger wrote:
> How do we factor out such code? I prefer macros for simple function
> wrappers and functions for multiline sequences.

As we are working with hardware we should avoid any macros for function 
replacement. There are ugly side effects. Refer the getCx86()/setCx86() 
disaster in the kernel for Geode-GX1 chipset programming:

What wrong with that?

#define getCx86(reg) ({ outb((reg), 0x22); inb(0x23); })
#define setCx86(reg, data) do { \
       outb((reg), 0x22); \
       outb((data), 0x23); \
} while (0)

This works:
  x = getCx86(0x20);
  x |= 0x01;
  setCx86(0x20, x);

This fails badly:
  setCx86(0x20, getCx86(0x20) | 0x01);

getCx86 and setCx86 defined as inline functions are working in all cases. So: 
Do not use macros as functions.

Juergen

[ron minnich]

On Jan 6, 2008 1:43 PM, Juergen Beisert  wrote:
> getCx86 and setCx86 defined as inline functions are working in all cases. So:
> Do not use macros as functions.

That's a good point. It's easy to get fooled by macros.

Thanks

ron

[Carl-Daniel Hailfinger]

On 06.01.2008 23:41, ron minnich wrote:
> On Jan 6, 2008 1:43 PM, Juergen Beisert  wrote:
> > getCx86 and setCx86 defined as inline functions are working in all cases. So:
> > Do not use macros as functions.
>
> That's a good point. It's easy to get fooled by macros.

Macros have one big advantage as function wrappers, though: Their
arguments do not have to contain all arguments to the wrapped functions.
However, a macros MUST be easily visible as such to avoid "clever" usage.

Regards,
Carl-Daniel

[Russell Whitaker]

On Sun, 6 Jan 2008, ron minnich wrote:
> On Jan 6, 2008 1:43 PM, Juergen Beisert  wrote:
> > getCx86 and setCx86 defined as inline functions are working in all cases. So:
> > Do not use macros as functions.
>
> That's a good point. It's easy to get fooled by macros.

How about something a little more simple-minded yet keeping it readable?

Thus:

#define  EDGE  MP_IRQ_TRIGGER_EDGE
#define  LEVEL MP_IRQ_TRIGGER_LEVEL
#define  HIGH  MP_IRQ_POLARITY_HIGH
#define  LOW   MP_IRO_PLOARITY_LOW

Russ