Hi Detlev,
On 17 January 2014 08:13, Detlev Zundel dzu@denx.de wrote:
Hi Simon,
[...]
I think the Linux code has two big advantages - for one, we increase the overlap with Linux kernel proper and secondly we keep the 'grep'ability of the names which I really missed in your proposal.
Yes that seems reasonable.
Ok, I'm glad we are in agreement.
Many of U-Boot's options are not just yes/no, so I'm not sure what will happen with those. Perhaps they just can't be used with this macro? Part of my intent was to allow any option to be used.
In those cases the defines then are a "shortcut" to using a boolean + a value and we can make it work by introducing this boolean? I have no idea of how much work this would be, but it might be worthwhile getting some real numbers to that.
So for example you can do this:
struct {
const char *str; u_int len; int retry; const char *conf; /* Configuration value */ } delaykey [] = { { str: getenv("bootdelaykey"), retry: 1, conf: autoconf_autoboot_delay_str() }, { str: getenv("bootdelaykey2"), retry: 1, conf: autoconf_autoboot_delay_str2() }, { str: getenv("bootstopkey"), retry: 0, conf: autoconf_autoboot_stop_str() }, { str: getenv("bootstopkey2"), retry: 0, conf: autoconf_autoboot_stop_str2() }, };
or this:
/* don't retry auto boot? */
if (autoconf_boot_retry_time() && !delaykey[i].retry) retry_time = -1;
Note that autoconf_boot_retry_time() will return 0 if the CONFIG is not defined, or if its value is 0.
I'm having real trouble figuring out what this would do on first sight. Of course you could call me lazy, but by experience I tend to favour solutions that do not need geniuses to understand ;)
Well it is simply that we currently have options which may or may not be defined. If they are not defined, then it is an error to refer to them, so they must be guarded by an #ifdef. By defining them to 0 when not defined, we can avoid that guard.
Ok, I see. But in this way we are actually shutting up the compiler on code paths that we did not have before. This in effect is a "rather be quiet than error" strategy and I'm not sure that I want to use that when doing such changes. Call me old-fashioned, but I'd prefer to throw an error and fix it after thinking it through from todays perspective
(I can say this easily if I'm not the one who has to do the fixes ;)
Well another approach would be to change the meaning of options like CONFIG_BOOTDELAY:
- Boot Delay: CONFIG_BOOTDELAY - in seconds Delay before automatically booting the default image; set to -1 to disable autoboot. set to -2 to autoboot with no delay and not check for abort (even when CONFIG_ZERO_BOOTDELAY_CHECK is defined).
Here it is enabled if >= 0, disabled if -1 and -2 for another option. If it is not defined at all then it is effectively -1, meaning disabled. IMO these could be separated out a little better. The problem here is that we can't have code like:
if (CONFIG_BOOTDELAY != -1) { // do some bootdelay processing }
as we will get a compile error if CONFIG_BOOTDELAY is not defined. In this case, CONFIG_BOOTDELAY being undefined is equivalent to defining it to -1 (I think).
There are only a small number of options in this category, but even with one, it prevents the technique above.
It seems to me we should provide the Linux feature in U-Boot as part of the Kconfig stuff, and see where it takes us. Combined with a bit more rationalisation of things like main.c it might be enough.
Why not reimplement your patch set along those lines? I still really would _love_ to see us using the compiler more to check for errors and reduce the number of "potential source code combination" that we have.
Well certainly I could, but I did not want to do this while Kbuild/Kconfig is in progress, and we are not quite clear on what to do. I think Kbuild is done - we will probably get the Linux autoconf stuff for free with Kconfig which I understand is coming very soon.
This makes a lot of sense yes. Actually I'm pretty excited about this excellent and continuous work from Masahiro-san on that topic!
Yes, looking forward to an early merge!
After that I can certainly take another look at main.c and see how it can be improved.
Sure, its only that I wanted to keep the ball rolling as I really believe the wins to be had from such a change are substantial for our codebase.
Certainly main.c could use something like this as my example showed. In fact it might be nice to split out the parser into a separate file.
Regards, Simon