Macro inflation [was: Re: change log for rtems (2010-09-08)]

[Ralf Corsepius]

On 09/10/2010 09:18 AM, Sebastian Huber wrote:
> On 09/10/2010 08:45 AM, Thomas Dörfler wrote:
>> Eric, Ralf,
>>
>> obviously we have different opinions here. Once again: I agree that a
>> different namespace is required for these definitions, but I think the
>> macros _do_ improve readability.
>>
>> I would like to move the discussion back to the rtems-vc mailing list to
>> allow Sebastian to express his opinions.
>>
>> wkr,
>>
>> Thomas.
>>
>> On 09.09.2010 15:43, Eric Norum wrote:
>>> On Sep 9, 2010, at 2:40 AM, Ralf Corsepius wrote:
>>>
>>>> Hi,
>>>>
>>>> Sebastian Huber committed the patch to CVS, yesterday. Unfortunately nobody so far has responded to my remark.
>>>>
>>>> Therefore, I don't see another alternative but to bring this to the attention of the SC.
>>>>
>>>> Rationale:
>>>>
>>>> a) this patch is bad code and must be removed.
>>>> The macros are not namespace safe and thus are likely to clash with other macros. E.g. using a macro called BIT8 is grossly negligant.
>
> Yes, it is not name space safe.  There is no reason for this.

Rubbish. Your file is a global header and can be included from any 
source file anywhere. It's mere random lack these macros currently don't 
conflict.

> This file is
> intended to simplify the description of flags and fields from hardware
> registers.  These definitions are only useful for a limited scope.  If you put
> the hardware dependent definitions into a wider scope, e.g. via bsp.h you do
> something wrong.
>
> Also preprocessor stuff doesn't show up in the symbol table.
Symbol tables are not of any relevance here.

The probem is the code such macros produce after preprocessing and the 
issues such macros introduce when going after bugs.

> You can find such macros everywhere in the source code.
Correct. RTEMS has an unfortunate history of applying the macro paradigm.

>  They are nothing new.
And we have repeatedly be hit by them.

Such macros' potential brokenenesses are very hard to catch. They often 
hang around undiscovered for years until somebody trips over them.

>   It is only a collection of them in a separate header file.
>
>>>>
>>>> b) I fundamentally question the usefulness and the correctness of this file and consider this code to be dirty.
>
> If you don't like this header file, don't use it.
>
>>>>
>>>>
>>>> Ralf
>>>
>>>
>>> I'm 100% in agreement with Ralf here.
>>> Namespace pollution is a serious issue.
>>> The macros don't do anything to improve readability of the code in which they are used.
>
> Making it really easy to write, read and verify flag and field definitions for
> big-endian machines from Motorola and Freescale manuals is not an improvement?
>
>>> If someone doesn't understand the C idioms for bit-set, bit-test, bit-clear, etc. they probably shouldn't be trying to program device I/O routines.
>
> Do you claim that someone who uses such macros doesn't understand the C idioms
> for bit-set, bit-test, bit-clear, etc.?
>
> Example (MPC5200 - ICTL External Enable and External Types Register):
>
> #define ICTL_EET_ECLR0 BBIT32(4)
> #define ICTL_EET_ECLR1 BBIT32(5)
> #define ICTL_EET_ECLR2 BBIT32(6)
> #define ICTL_EET_ECLR3 BBIT32(7)
> #define ICTL_EET_ETYPE0(val) BFIELD32(val, 8, 9)
> #define ICTL_EET_ETYPE1(val) BFIELD32(val, 10, 11)
> #define ICTL_EET_ETYPE2(val) BFIELD32(val, 12, 13)
> #define ICTL_EET_ETYPE3(val) BFIELD32(val, 14, 15)
> #define ICTL_EET_SET_ETYPE0(reg, val) BSETFIELD32(reg, val, 8, 9)
> #define ICTL_EET_SET_ETYPE1(reg, val) BSETFIELD32(reg, val, 10, 11)
> #define ICTL_EET_SET_ETYPE2(reg, val) BSETFIELD32(reg, val, 12, 13)
> #define ICTL_EET_SET_ETYPE3(reg, val) BSETFIELD32(reg, val, 14, 15)
> #define ICTL_EET_MEE BBIT32(19)
> #define ICTL_EET_EENA0 BBIT32(20)
> #define ICTL_EET_EENA1 BBIT32(21)
> #define ICTL_EET_EENA2 BBIT32(22)
> #define ICTL_EET_EENA3 BBIT32(23)
> #define ICTL_EET_CEB BBIT32(31)
>
> Please write this without these macros and compare the results.

These are very specialized _private_ macros.

What you did, is to add global macros to a global header.

Ralf