Hi Wolfgang, On Wed, 06 Nov 2013 08:24:44 +0100, Wolfgang Denk <w...@denx.de> wrote:
> Dear Simon Glass, > > In message <1382800457-26608-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 > > 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? Agreed on the whole -- plus, introducing indentation in configuration option testing will make it easier to spot and understand nested configuration conditionals. > Best regards, > > Wolfgang Denk > Amicalement, -- Albert. _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot