[U-Boot] [PATCH v4 0/8] Provide a mechanism to avoid using #ifdef everywhere

[Simon Glass]

Hi Detlev,

On 17 January 2014 08:13, Detlev Zundel <dzu at 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