Hi Tom,
On Mon, Feb 18, 2013 at 1:36 PM, Tom Rini trini@ti.com wrote:
-----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.
So config_bootdelay(), or cfg_bootdelay() for the value, and build_has_bootdelay() for the enable?
[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).
Possibly, but it seems like the biggest impact is the slower reconfigure - sed runs for several seconds.
[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.
I suspect cleaning up main.c would involve a number of patches in the end.
BTW this patch also is interesting from the point of view of the generic board series. At the moment I am using a list of function pointers, which is quite nice, but does defeat the compiler optimisations, so adds about 1KB of code size. Also it's hard to imagine removing the #ifdefs from a list of function pointers using this mechanism.
Regards, Simon
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-----