From mboxrd@z Thu Jan 1 00:00:00 1970 From: Tom Rini Date: Mon, 18 Feb 2013 16:36:22 -0500 Subject: [U-Boot] [RFC PATCH] Provide a mechanism to avoid using #ifdef everywhere In-Reply-To: <20130218192348.7AB75200531@gemini.denx.de> References: <1361207920-24983-1-git-send-email-sjg@chromium.org> <20130218192348.7AB75200531@gemini.denx.de> Message-ID: <51229ED6.4080604@ti.com> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de -----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 -----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-----