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

[Albert ARIBAUD]

Hi Wolfgang,

On Wed, 06 Nov 2013 08:24:44 +0100, 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?

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.