[U-Boot] [RFC PATCH] Provide a mechanism to avoid using #ifdef everywhere

[Tom Rini]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 02/18/2013 02:23 PM, Wolfgang Denk wrote:
> Dear Simon,
> 
> In message <1361207920-24983-1-git-send-email-sjg at chromium.org> you
> wrote:
>> Many parts of the U-Boot code base are sprinkled with #ifdefs.
>> This makes different boards compile different versions of the
>> source code, meaning that we must build all boards to check for
>> failures. It is easy to misspell
> ...
>> +# Create a C header file where every '#define CONFIG_XXX value'
>> becomes +# '#define config_xxx() value', or '#define config_xxx()
>> 0' where the CONFIG +# is not used by this board configuration.
>> This allows C code to do things +# like 'if (config_xxx())' and
>> have the compiler remove the dead code, +# instead of using
>> '#ifdef CONFIG_XXX...#endif'. Note that in most cases +# if the
>> config_...() returns 0 then the option is not enabled. In some
>> rare +# cases such as CONFIG_BOOTDELAY, the config can be enabled
>> but still have a +# a value of 0. So in addition we a #define
>> config_xxx_enabled(), setting the +# value to 0 if the option is
>> disabled, 1 if enabled. This last feature will +# hopefully be
>> deprecated soon.
> 
> I think config_* is not a good name to use here - it has never been
> a reserved prefix so far, so it is IMO a bad idea to turn it into
> one now .  We already have quite a number such variable names in
> the code all over the place (not sure I caught them all):

Indeed.  It's not a good choice to suddenly reserve now either.
build_has_xxx() ?  I'm not sure.

[snip]
>> +The compiler will see that config_xxx() evalutes to a constant
>> and will +eliminate the dead code. The resulting code (and code
>> size) is the same.
> 
> Did you measure the impact on compile time?

Probably won't have a good handle on this without converting a whole
build target's needs (just about).

[snip]
>> -#if defined CONFIG_ZERO_BOOTDELAY_CHECK /* * Check if key
>> already pressed * Don't check if bootdelay < 0 */ -	if (bootdelay
>> >= 0) { +	if (config_zero_bootdelay_check() && bootdelay >= 0) { 
>> if (tstc()) {	/* we got a key press	*/ (void) getc();  /* consume
>> input	*/ puts ("\b\b\b 0"); abort = 1;	/* don't auto boot	*/ } } 
>> -#endif
> 
> I think code like this needs more careful editing.
> 
> With the #ifdef, it was clear that the comment only applies if 
> CONFIG_ZERO_BOOTDELAY_CHECK is defined, but now it suddenly
> becomes valid _always_, which is pretty much misleading to someone
> trying to understand this code.   I think it would be necessary to
> rephrase the commend, and move it partially into the if() block.

Exactly.  This type of change will require a lot of re-commenting to
make it clear what's going on now, and after the change even more-so.

- -- 
Tom
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iQIcBAEBAgAGBQJRIp7WAAoJENk4IS6UOR1W1S0QAKzMt8KkcVdRTFElSjlze35P
PxFkO/W8YchPkzwMdhpU4UxHNYoFziLk5pLfj8hhh9uyQ7Lp0I411PZtJ+mkt3I5
RQf8xPHF9PDN2w0ZsxYKd0JE9LgFB/b9EmOOpzxy7Bge3aEGrfnhqgjNgnPGgVgO
eCcLGZLrGYlbcL9SOJNxBdFjgCxJvRfrNtsaLOJc5SveeqMNGISp6xc5WDWnmf1f
JAHNg7d9ik5d782AC76jbNUVOIp+85N0dMjhCdLR4YZBdNTC5grAW6x7gEGj+vYZ
xUE2/Y20FG1Ie3vQjbbW1gUhYtxCxjFLl+UkUOn5bf6F0eDgyUvaSt17nrO3GSbQ
hgunWaw9fZoVKMhb1WNnRHmLU5gS9rVlYGGsGibiMs1VPuiYpTLM/kuxfVitxJO0
Ysgkyotgfj2RbnKuBCkMVmvxBzIC3S2bAtxY97TwVrpEh2ZB7r2YwEKak8WhVxyQ
nMyMulgpZoMJLM3fJEcF/kQPIzsKtz1Fl/YQXGZlI2lQpohE03kAPVQDyVeTQpGA
FzGOUwVZY4WbcKrpcCV4tJOEnHRVyDR8ntQx0udMjtChaLE40fAmmUlWmWrnE4yV
SVBM+PS2d7NCXt85QpS0eMc/UdFI0v1i6R24KEHEfqQe1WEdQb1wC7XXYblutZ8z
ySlnbeEMfeDg5i5FHX46
=CF0y
-----END PGP SIGNATURE-----