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

[Simon Glass]

Hi Detlev,

On 14 January 2014 03:11, Detlev Zundel <dzu at denx.de> wrote:
> Hi Simon,
>
> as I don't see any follow-up, allow me to jump in here even that late :)
>
>> Hi Wolfgang,
>>
>> On Wed, Nov 6, 2013 at 1:24 AM, Wolfgang Denk <wd at denx.de> wrote:
>>> Dear Simon Glass,
>>>
>>> In message <1382800457-26608-1-git-send-email-sjg at 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
>>>> an #ifdef and there is not as much checking of this by the compiler.
>> Multiple
>>>> dependent #ifdefs are harder to do than with if..then..else. Variable
>>>> declarations must be #idefed as well as the code that uses them, often
>> much
>>>> later in the file/function. #ifdef indents don't match code indents and
>>>> have their own separate indent feature. Overall, excessive use of #idef
>>>> hurts readability and makes the code harder to modify and refactor. For
>>>> people coming newly into the code base, #ifdefs can be a big barrier.
>>>>
>>>> The use of #ifdef in U-Boot has possibly got a little out of hand. In an
>>>> attempt to turn the tide, this series includes a patch which provides a
>> way
>>>> to make CONFIG macros available to C code without using the preprocessor.
>>>> This makes it possible to use standard C conditional features such as
>>>> if/then instead of #ifdef. A README update exhorts compliance.
>>>
>>> As mentioned before, I'm really interested in seeing something like
>>> this going into mainline, but I have some doubts about the actual
>>> implementation.
>>>
>>> To summarize:  Your current proposal was to convert code snippets
>>> like this:
>>>
>>>         #ifdef CONFIG_VERSION_VARIABLE
>>>                 setenv("ver", version_string);  /* set version variable */
>>>         #endif
>>>
>>> into
>>>
>>>         if (autoconf_version_variable())
>>>                 setenv("ver", version_string);  /* set version variable */
>>>
>>> By chance I ran about "include/linux/kconfig.h" in the Linux kernel
>>> tree, which provides (among other things) the IS_ENABLED() macro that
>>> implements essentially the very same feature.  Using this, the same
>>> code would be written as:
>>>
>>>         if (IS_ENABLED(CONFIG_VERSION_VARIABLE))
>>>                 setenv("ver", version_string);  /* set version variable */
>>>
>>> I agree that this does not solve some of the isses that have been
>>> raised about this change (indentation level increses - which may in
>>> turn require reformatting of bigger parts of the code; code becomes
>>> less readable), but on the other hand it avoids the need for a new
>>> autoconf header file, and it should be possible to introduce this
>>> easily step by step.
>>>
>>> And I really like the idea of re-using existing code that is already
>>> known to Linux hackers, especially as we we are currently having our
>>> eyes on the Kconfig stuff anyway.
>>>
>>>
>>> What do you think?
>>
>> Fair enough, sounds reasonable. The relevant kernel code is quite unusual...
>>
>> /*
>>>  * Helper macros to use CONFIG_ options in C/CPP expressions. Note that
>>>  * these only work with boolean and tristate options.
>>>  */
>>> /*
>>>  * Getting something that works in C and CPP for an arg that may or may
>>>  * not be defined is tricky.  Here, if we have "#define CONFIG_BOOGER 1"
>>>  * we match on the placeholder define, insert the "0," for arg1 and
>>> generate
>>>  * the triplet (0, 1, 0).  Then the last step cherry picks the 2nd arg (a
>>> one).
>>>  * When CONFIG_BOOGER is not defined, we generate a (... 1, 0) pair, and
>>> when
>>>  * the last step cherry picks the 2nd arg, we get a zero.
>>>  */
>>> #define __ARG_PLACEHOLDER_1 0,
>>> #define config_enabled(cfg) _config_enabled(cfg)
>>> #define _config_enabled(value) __config_enabled(__ARG_PLACEHOLDER_##value)
>>> #define __config_enabled(arg1_or_junk) ___config_enabled(arg1_or_junk 1, 0)
>>> #define ___config_enabled(__ignored, val, ...) val
>>> /*
>>>  * IS_ENABLED(CONFIG_FOO) evaluates to 1 if CONFIG_FOO is set to 'y' or
>>> 'm',
>>>  * 0 otherwise.
>>>  *
>>>  */
>>> #define IS_ENABLED(option) \
>>> (config_enabled(option) || config_enabled(option##_MODULE))
>>
>
> 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.

>
>> 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.

>
>> 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.

After that I can certainly take another look at main.c and see how it
can be improved.

>
> Thanks for all the work so far!
>   Detlev
>
> --
> The  C++  STL, with its dyslexia-inducing syntax blizzard of colons and angle
> brackets, guarantees that if you try to declare any reasonable data structure,
> your first seven attempts will result in compiler errors of Wagnerian fierce-
> ness.                         -- James Mickens

Hard to disagree with this.

> --
> DENX Software Engineering GmbH,      MD: Wolfgang Denk & Detlev Zundel
> HRB 165235 Munich,  Office: Kirchenstr.5, D-82194 Groebenzell, Germany
> Phone: (+49)-8142-66989-40 Fax: (+49)-8142-66989-80 Email: dzu at denx.de

Regards,
Simon