-----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@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