Re: [U-Boot] [PATCH v4 0/8] Provide a mechanism to avoid using #ifdef everywhere
Hi Detlev,
On 17 January 2014 08:13, Detlev Zundel d...@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
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v4 0/8] Provide a mechanism to avoid using #ifdef everywhere
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 ;)
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!
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.
Cheers
Detlev
--
Golden rule #12: When the comments do not match the code, they probably are
both wrong. -- Steven Rostedt 1300126962.9910.128.ca...@gandalf.stny.rr.com
--
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: d...@denx.de
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v4 0/8] Provide a mechanism to avoid using #ifdef everywhere
Hi Detlev,
On 14 January 2014 03:11, Detlev Zundel d...@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 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?
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() },
Re: [U-Boot] [PATCH v4 0/8] Provide a mechanism to avoid using #ifdef everywhere
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 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?
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.
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),
Re: [U-Boot] [PATCH v4 0/8] Provide a mechanism to avoid using #ifdef everywhere
Dear Simon,
In message capnjgz3b4szi_2h2sw7oop932ccgou8ho5e_fkmjcyv4ws+...@mail.gmail.com
you wrote:
...
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 */
...
Fair enough, sounds reasonable. The relevant kernel code is quite unusual...
Indeed, this code is really a bit creepy...
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.
I see. Um... But then, I think the usage of the macro is somewhat
dangerous anyway.
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() },
};
Well, this is not exactly easy to read either.
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 see what yo mean, but I'd rather clean up the code to avoid such
ambiguities.
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.
Sounds like a plan - but we don't have to wait; we can add the header
file any time we ike, and start converting pieces of code that seem to
deserve such treatment. And, probably even more interesting, we can
request it to be used for any new code being added.
I like the simplicity and power of the autoconf feature, but I have similar
reservations to others.
I understand you. Eventuyally ther eis no easy solution that
satisfies all.
Best regards,
Wolfgang Denk
--
DENX Software Engineering GmbH, MD: Wolfgang Denk Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: w...@denx.de
Dealing with failure is easy: work hard to improve. Success is also
easy to handle: you've solved the wrong problem. Work hard to
improve.
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v4 0/8] Provide a mechanism to avoid using #ifdef everywhere
Hi Wolfgang,
On Wed, Nov 6, 2013 at 1:24 AM, 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?
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))
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.
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.
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.
I like the simplicity and power of the autoconf
Re: [U-Boot] [PATCH v4 0/8] Provide a mechanism to avoid using #ifdef everywhere
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
Re: [U-Boot] [PATCH v4 0/8] Provide a mechanism to avoid using #ifdef everywhere
Hi Wolfgang, On 06.11.2013 08:24, Wolfgang Denk wrote: snip 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. I just recently also noticed this IS_ENABLED() feature (in barebox btw) and thought directly about Simon's patchset regarding this matter. And I personally would favor IS_ENABLED() to the newly created autoconf_xxx names. Thanks, Stefan ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v4 0/8] Provide a mechanism to avoid using #ifdef everywhere
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? Best regards, Wolfgang Denk -- DENX Software Engineering GmbH, MD: Wolfgang Denk Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: w...@denx.de PROGRAM - n. A magic spell cast over a computer allowing it to turn one's input into error messages. v. tr. - To engage in a pastime similar to banging one's head against a wall, but with fewer opportunities for reward. ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v4 0/8] Provide a mechanism to avoid using #ifdef everywhere
Dear Simon, 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. While I agree in general with the goal and the implementation, there is an implementation detail I dislike - this is that the new code is harder to type and to read - I mean, something like CONFIG_SYS_HUSH_PARSER is already clumsy enough, but making this autoconf_sys_hush_parser() appears to be worse. Also, the connection to a CONFIG_* option is not easily visible. I also had feedback from Detlev (who is unfortunately on a business trip again so he didn't find time [yet] to comment himself); he commented that he really likes the idea, but does not like that we now have to access the well-known contants using a new name. Maybe we can find a shorter / easier to read way to do this - I think it would be really nice if we could see the well-known names. Best regards, Wolfgang Denk -- DENX Software Engineering GmbH, MD: Wolfgang Denk Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: w...@denx.de Where there's no emotion, there's no motive for violence. -- Spock, Dagger of the Mind, stardate 2715.1 ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v4 0/8] Provide a mechanism to avoid using #ifdef everywhere
On Mon, Oct 28, 2013 at 03:44:01PM +0100, Wolfgang Denk wrote:
Dear Simon,
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.
While I agree in general with the goal and the implementation, there
is an implementation detail I dislike - this is that the new code
is harder to type and to read - I mean, something like
CONFIG_SYS_HUSH_PARSER is already clumsy enough, but making this
autoconf_sys_hush_parser() appears to be worse. Also, the connection
to a CONFIG_* option is not easily visible.
I also had feedback from Detlev (who is unfortunately on a business
trip again so he didn't find time [yet] to comment himself); he
commented that he really likes the idea, but does not like that we now
have to access the well-known contants using a new name.
Maybe we can find a shorter / easier to read way to do this - I think
it would be really nice if we could see the well-known names.
I guess this is what I get for not being like Linus sometimes[1]. I think
what this series highlights is that we have a lot of code in
common/main.c that needs either re-thinking, re-factoring or splitting
out into difference functions and files. And when we're turning
#ifdef CONFIG_FOO
... whatever ...
#endif
into
if (autoconf_foo()) {
... whatever ...
}
The code starts looking worse in some cases since we're already 3
indents in. The problem is we have lots of ifdef code, in some areas of
the code. This hides the problem, not fixes the problem.
Looking over the first parts of the series, weak functions for example
would help in a number of cases, especially if we split things out of
main.c and into other files too.
--
Tom
[1]: By which I mean being very forceful when saying I don't like an
idea.
signature.asc
Description: Digital signature
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v4 0/8] Provide a mechanism to avoid using #ifdef everywhere
Hi Tom,
On Mon, Oct 28, 2013 at 9:32 AM, Tom Rini tr...@ti.com wrote:
On Mon, Oct 28, 2013 at 03:44:01PM +0100, Wolfgang Denk wrote:
Dear Simon,
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.
While I agree in general with the goal and the implementation, there
is an implementation detail I dislike - this is that the new code
is harder to type and to read - I mean, something like
CONFIG_SYS_HUSH_PARSER is already clumsy enough, but making this
autoconf_sys_hush_parser() appears to be worse. Also, the connection
to a CONFIG_* option is not easily visible.
I also had feedback from Detlev (who is unfortunately on a business
trip again so he didn't find time [yet] to comment himself); he
commented that he really likes the idea, but does not like that we now
have to access the well-known contants using a new name.
Maybe we can find a shorter / easier to read way to do this - I think
it would be really nice if we could see the well-known names.
I guess this is what I get for not being like Linus sometimes[1]. I think
what this series highlights is that we have a lot of code in
common/main.c that needs either re-thinking, re-factoring or splitting
out into difference functions and files. And when we're turning
#ifdef CONFIG_FOO
... whatever ...
#endif
into
if (autoconf_foo()) {
... whatever ...
}
The code starts looking worse in some cases since we're already 3
indents in. The problem is we have lots of ifdef code, in some areas of
the code. This hides the problem, not fixes the problem.
While I agree with this, the question is more whether using the
compiler instead of the preprocessor helps with reducing the number of
code paths and the number of boards we must build to test things. In
many cases the CONFIG options hide features that we don't want to
enable.
I did a series that improved main.c in various ways including
splitting the code differently - there are a few more things that
could be done, but it will still be a mountain of #ifdefs.
I would quite like to split the command editing stuff into its own
file - in fact main.c could be split into several files:
- command line
- command editing
- parser
But does this help? I wasn't entirely sure.
Looking over the first parts of the series, weak functions for example
would help in a number of cases, especially if we split things out of
main.c and into other files too.
I am not a huge fan of weak functions since it isn't clear what
happens when they are called. But again if you have a specific example
it would help me understand your intent here.
Regards,
Simon
--
Tom
[1]: By which I mean being very forceful when saying I don't like an
idea.
BTW I am not sure about this idea either - let's figure out exactly
what it can help with and whether it is worth it. Perhaps main.c was
not the best choice - but board files have loads of #ifdefs also.
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
[U-Boot] [PATCH v4 0/8] Provide a mechanism to avoid using #ifdef everywhere
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 an example of how to use this, this series replaces all but two #ifdefs from the main code body of common/main.c, which is one of the largest users of #ifdef, even after a recent cleanup: $ for f in $(find . -name *.c); do echo $(grep -c ifdef $f) $f; \ done |sort -nr |head 82 ./common/board_r.c 57 ./common/board_f.c 57 ./arch/powerpc/cpu/mpc83xx/cpu_init.c 49 ./common/main.c 49 ./arch/powerpc/lib/board.c 45 ./drivers/video/cfb_console.c 40 ./drivers/mtd/cfi_flash.c 38 ./net/tftp.c 37 ./drivers/usb/host/ohci-hcd.c 36 ./drivers/fpga/ivm_core.c Code size for this series seems to be roughly neutral (below numbers are average change in byte size for each region: blackfin: (for 21/35 boards) all -13.7 text -13.7 x86: (for 1/1 boards) bss +4.0 data +4.0 text -8.0 avr32: (for 10/10 boards) all -7.2 data -2.0 text -5.2 m68k: (for 35/50 boards) all -30.9 text -30.9 powerpc: (for 673/675 boards) all -20.8 bss +0.4 data +0.0 rodata -0.7 spl/u-boot-spl:all +0.1 spl/u-boot-spl:data +0.1 text -20.5 sandbox: (for 1/1 boards) all +16.0 bss +16.0 sh: (for 16/21 boards) all -76.2 bss +3.2 rodata -15.5 text -64.0 nios2: (for 3/3 boards) all +18.7 bss +1.3 data -1.3 text +18.7 microblaze: (for 1/1 boards) all -864.0 bss +8.0 rodata -216.0 text -656.0 arm: (for 286/344 boards) all -51.7 bss -5.7 data +0.2 rodata -3.9 spl/u-boot-spl:all +0.2 spl/u-boot-spl:bss +0.2 text -42.4 nds32: (for 3/3 boards) all -52.0 text -52.0 Note that a config_drop.h file is added - this defines all the CONFIGs which are not used in any board config file. Without this, autoconf cannot define the macros for this CONFIGs. Compile time for main.c does not seem to be any different in my tests. The time to perform the 'dep' step (which now creates autoconf.h) increases, from about 2.8s to about 4.6s. This additional time is used to grep, sed and sort the contents of all the header file in U-Boot. The time for an incremental build is not affected. It would be much more efficient to maintain a list of all available CONFIG defines, but no such list exists at present. Buildman output shows no additional failures from mainline (commit 02 is a bug in buildman and commit 11 will be sent separately): $ ./tools/buildman/buildman -b us-config7 -s Summary of 10 commits for 1178 boards (32 threads, 1 job per thread) 01: usb: rename board_usb_init_type to usb_init_type blackfin: + bf561-acvilon cm-bf561 blackstamp br4 bct-brettl2 cm-bf527 dnp5370 bf506f-ezkit ip04 bf527-sdp bf609-ezkit bf537-stamp bf527-ezkit-v2 cm-bf537e tcm-bf518 cm-bf537u bf537-pnav cm-bf533 pr1 bf533-ezkit ibf-dsp561 bf537-srv1 cm-bf548 bf537-minotaur bf538f-ezkit bf548-ezkit bf525-ucr2 blackvme bf527-ezkit tcm-bf537 bf533-stamp bf518f-ezbrd bf527-ad7160-eval bf526-ezbrd bf561-ezkit m68k: + M54455EVB_a66 M5329AFEE M5249EVB idmr M5208EVBE eb_cpu5282 M5475FFE M54451EVB astro_mcf5373l M54418TWR_serial_rmii M54455EVB_intel M5282EVB M54455EVB_i66 M5475GFE M5253DEMO M54455EVB_stm33 M5485BFE M5485DFE TASREG M5329BFEE M52277EVB M5475EFE M5475CFE cobra5272 M5485AFE M53017EVB M5485HFE M5235EVB M5253EVBE M54418TWR_nand_mii M54418TWR_nand_rmii_lowfreq M5475BFE M5475DFE M5275EVB M52277EVB_stmicro eb_cpu5282_internal M54451EVB_stmicro M5271EVB M5485GFE M5485EFE M5485FFE M54418TWR M5235EVB_Flash32 M5373EVB M54418TWR_nand_rmii M54418TWR_serial_mii M5485CFE M54455EVB M5475AFE M5272C3 powerpc: + MVBLM7 MVSMR lcd4_lwmon5 sparc: + grsim grsim_leon2 gr_cpci_ax2000 gr_xc3s_1500 gr_ep2s60 sh: + rsk7269 rsk7264 sh7752evb rsk7203 sh7757lcr microblaze: + microblaze-generic openrisc: + openrisc-generic arm: + pm9g45 qong palmtc zipitz2 omap3_zoom1 omap3_overo goflexhome davinci_sonata VCMA9 iconnect km_kirkwood_pci ib62x0 lubbock ethernut5 zynq_dcc vpac270_nor_128
Re: [U-Boot] [PATCH v4 0/8] Provide a mechanism to avoid using #ifdef everywhere
Hi Simon, On Mon, 24 Jun 2013 17:52:03 -0700, Simon Glass s...@chromium.org wrote: Hi Albert, On Sun, Jun 23, 2013 at 12:29 AM, Albert ARIBAUD albert.u.b...@aribaud.netwrote: Hi Simon, On Mon, 17 Jun 2013 07:44:52 -0700, Simon Glass s...@chromium.org wrote: Note that a config_drop.h file is added - this defines all the CONFIGs which are not used in any board config file. Without this, autoconf cannot define the macros for this CONFIGs. Compile time for main.c does not seem to be any different in my tests. The time to perform the 'dep' step (which now creates autoconf.h) increases, from about 2.8s to about 4.6s. This additional time is used to grep, sed and sort the contents of all the header file in U-Boot. The time for an incremental build is not affected. It would be much more efficient to maintain a list of all available CONFIG defines, but no such list exists at present. Stop me if I am wrong, but do you not have this list already, since at one point you grep, sed and sort the whole list of config options, then at another point generate the list of unused ones? Well yes I create the list. But I don't 'have' it in the sense that it is a pre-existing file in the tree. My point was that if the file existed I would not need to create it in the build system. I asked about this at one point, and the comment was made that putting it in the source tree 'staticly' is risky, since someone might add a new option and it would not work. Perhaps when Kconfig is in there things will be different. Understod. Granted, that's the list of config options defined, not necessarily the list of options used, but a second variation of the grep/sed/sort might give you a hint on that. Plus, I would love having scripts in tools/ that look for either defined or used config options. With this series you kind-of get this feature - you can look at the files it creates along the way. What I meant is, this patch creates those lists with scripts. I'd like these scripts to be available to the developer in tools/ so that anyone can regenerate the lists (and do this only) at any time. Regards, Simon Amicalement, -- Albert. ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v4 0/8] Provide a mechanism to avoid using #ifdef everywhere
Hi Albert, On Thu, Jun 27, 2013 at 12:04 AM, Albert ARIBAUD albert.u.b...@aribaud.netwrote: Hi Simon, On Mon, 24 Jun 2013 17:52:03 -0700, Simon Glass s...@chromium.org wrote: Hi Albert, On Sun, Jun 23, 2013 at 12:29 AM, Albert ARIBAUD albert.u.b...@aribaud.netwrote: Hi Simon, On Mon, 17 Jun 2013 07:44:52 -0700, Simon Glass s...@chromium.org wrote: Note that a config_drop.h file is added - this defines all the CONFIGs which are not used in any board config file. Without this, autoconf cannot define the macros for this CONFIGs. Compile time for main.c does not seem to be any different in my tests. The time to perform the 'dep' step (which now creates autoconf.h) increases, from about 2.8s to about 4.6s. This additional time is used to grep, sed and sort the contents of all the header file in U-Boot. The time for an incremental build is not affected. It would be much more efficient to maintain a list of all available CONFIG defines, but no such list exists at present. Stop me if I am wrong, but do you not have this list already, since at one point you grep, sed and sort the whole list of config options, then at another point generate the list of unused ones? Well yes I create the list. But I don't 'have' it in the sense that it is a pre-existing file in the tree. My point was that if the file existed I would not need to create it in the build system. I asked about this at one point, and the comment was made that putting it in the source tree 'staticly' is risky, since someone might add a new option and it would not work. Perhaps when Kconfig is in there things will be different. Understod. Granted, that's the list of config options defined, not necessarily the list of options used, but a second variation of the grep/sed/sort might give you a hint on that. Plus, I would love having scripts in tools/ that look for either defined or used config options. With this series you kind-of get this feature - you can look at the files it creates along the way. What I meant is, this patch creates those lists with scripts. I'd like these scripts to be available to the developer in tools/ so that anyone can regenerate the lists (and do this only) at any time. Yes, I figured that's what you were angling for. I will take a look. Regards, Simon ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v4 0/8] Provide a mechanism to avoid using #ifdef everywhere
Hi Albert, On Sun, Jun 23, 2013 at 12:29 AM, Albert ARIBAUD albert.u.b...@aribaud.netwrote: Hi Simon, On Mon, 17 Jun 2013 07:44:52 -0700, Simon Glass s...@chromium.org wrote: Note that a config_drop.h file is added - this defines all the CONFIGs which are not used in any board config file. Without this, autoconf cannot define the macros for this CONFIGs. Compile time for main.c does not seem to be any different in my tests. The time to perform the 'dep' step (which now creates autoconf.h) increases, from about 2.8s to about 4.6s. This additional time is used to grep, sed and sort the contents of all the header file in U-Boot. The time for an incremental build is not affected. It would be much more efficient to maintain a list of all available CONFIG defines, but no such list exists at present. Stop me if I am wrong, but do you not have this list already, since at one point you grep, sed and sort the whole list of config options, then at another point generate the list of unused ones? Well yes I create the list. But I don't 'have' it in the sense that it is a pre-existing file in the tree. My point was that if the file existed I would not need to create it in the build system. I asked about this at one point, and the comment was made that putting it in the source tree 'staticly' is risky, since someone might add a new option and it would not work. Perhaps when Kconfig is in there things will be different. Granted, that's the list of config options defined, not necessarily the list of options used, but a second variation of the grep/sed/sort might give you a hint on that. Plus, I would love having scripts in tools/ that look for either defined or used config options. With this series you kind-of get this feature - you can look at the files it creates along the way. Regards, Simon ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v4 0/8] Provide a mechanism to avoid using #ifdef everywhere
Hi Simon, On Mon, 17 Jun 2013 07:44:52 -0700, Simon Glass s...@chromium.org wrote: Note that a config_drop.h file is added - this defines all the CONFIGs which are not used in any board config file. Without this, autoconf cannot define the macros for this CONFIGs. Compile time for main.c does not seem to be any different in my tests. The time to perform the 'dep' step (which now creates autoconf.h) increases, from about 2.8s to about 4.6s. This additional time is used to grep, sed and sort the contents of all the header file in U-Boot. The time for an incremental build is not affected. It would be much more efficient to maintain a list of all available CONFIG defines, but no such list exists at present. Stop me if I am wrong, but do you not have this list already, since at one point you grep, sed and sort the whole list of config options, then at another point generate the list of unused ones? Granted, that's the list of config options defined, not necessarily the list of options used, but a second variation of the grep/sed/sort might give you a hint on that. Plus, I would love having scripts in tools/ that look for either defined or used config options. Amicalement, -- Albert. ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
[U-Boot] [PATCH v4 0/8] Provide a mechanism to avoid using #ifdef everywhere
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 an example of how to use this, this series replaces all but two #ifdefs from the main code body of common/main.c, which is one of the largest users of #ifdef, even after a recent cleanup: for f in $(find . -name *.c); do echo $(grep -c ifdef $f) $f; do ne |sort -nr |head 81 ./common/board_r.c 57 ./arch/powerpc/cpu/mpc83xx/cpu_init.c 55 ./common/board_f.c 49 ./common/main.c 48 ./arch/powerpc/lib/board.c 47 ./drivers/video/cfb_console.c 40 ./drivers/mtd/cfi_flash.c 38 ./net/tftp.c 37 ./drivers/usb/host/ohci-hcd.c 36 ./drivers/fpga/ivm_core.c Code size for this series seems to be roughly neutral (below numbers are average change in byte size for each region: blackfin: (for 24/35 boards) all -11.0 text -11.0 x86: (for 1/1 boards) bss +20.0 data +4.0 text -24.0 avr32: (for 10/10 boards) all -8.4 text -8.4 sandbox: (for 1/1 boards) all +16.0 bss +16.0 m68k: (for 41/50 boards) all -31.9 text -31.9 powerpc: (for 639/641 boards) all -20.5 bss +0.0 rodata -0.5 text -20.0 sparc: (for 5/5 boards) all -28.8 text -28.8 sh: (for 16/21 boards) all -78.2 bss +3.2 rodata -15.5 text -66.0 nios2: (for 3/3 boards) all +24.0 bss +1.3 data -1.3 text +24.0 arm: (for 307/327 boards) all -41.0 bss +3.5 data +0.1 rodata -3.6 spl/u-boot-spl:all -0.1 spl/u-boot-spl:bss -0.1 text -41.0 Note that a config_drop.h file is added - this defines all the CONFIGs which are not used in any board config file. Without this, autoconf cannot define the macros for this CONFIGs. Compile time for main.c does not seem to be any different in my tests. The time to perform the 'dep' step (which now creates autoconf.h) increases, from about 2.8s to about 4.6s. This additional time is used to grep, sed and sort the contents of all the header file in U-Boot. The time for an incremental build is not affected. It would be much more efficient to maintain a list of all available CONFIG defines, but no such list exists at present. Buildman output shows no additional failures from mainline: 01: Merge branch 'master' of git://www.denx.de/git/u-boot-mmc blackfin: + bf561-acvilon cm-bf561 blackstamp br4 bct-brettl2 cm-bf527 dnp5370 bf506f-ezkit ip04 bf527-sdp bf609-ezkit bf537-stamp bf527-ezkit-v2 cm-bf537e tcm-bf518 cm-bf537u bf527-ezkit bf537-pnav cm-bf533 pr1 bf533-ezkit ibf-dsp561 bf537-srv1 cm-bf548 bf537-minotaur bf538f-ezkit bf548-ezkit bf525-ucr2 blackvme tcm-bf537 bf533-stamp bf518f-ezbrd bf527-ad7160-eval bf526-ezbrd bf561-ezkit m68k: + M54455EVB_a66 M5329AFEE M5249EVB idmr M5208EVBE M5475FFE M54451EVB astro_mcf5373l M54418TWR_serial_rmii M54455EVB_intel M5282EVB M54455EVB_i66 M5475GFE M5253DEMO M54455EVB_stm33 M5485BFE M5485DFE M5329BFEE M52277EVB M5475EFE M5475CFE M5485AFE M53017EVB M5475AFE M5485HFE M5235EVB M5253EVBE M54418TWR_nand_mii M54418TWR_nand_rmii_lowfreq TASREG cobra5272 M5475BFE M5475DFE M5275EVB M52277EVB_stmicro eb_cpu5282 eb_cpu5282_internal M54451EVB_stmicro M5271EVB M5485GFE M5485EFE M5485FFE M54418TWR M5235EVB_Flash32 M5373EVB M54418TWR_nand_rmii M54418TWR_serial_mii M5485CFE M54455EVB M5272C3 powerpc: + MVBLM7 MVSMR lcd4_lwmon5 sh: + rsk7269 rsk7264 sh7757lcr sh7752evb rsk7203 microblaze: + microblaze-generic openrisc: + openrisc-generic arm: + palmtc zipitz2 VCMA9 lubbock zynq_dcc vpac270_nor_128 colibri_pxa270 kzm9g zynq xaeniax polaris pxa255_idp lp8x4x vpac270_ond_256 vpac270_nor_256 smdk2410 h2200 balloon3 palmld trizepsiv nds32: + adp-ag101p adp-ag102 adp-ag101 02: Implement autoconf header file 03: main: Use autoconf for boot retry feature 04: main: Remove CONFIG #ifdefs from the abortboot() code 05: main: Use autoconf to remove #ifdefs around process_boot_delay() 06: main: Use autoconf for boot_delay code 07: main: Use autoconf for parser selection 08: main: Use autoconf in command line
