Hi Tom,
On Mon, Feb 18, 2013 at 1:36 PM, Tom Rini <tr...@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-...@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-----
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
