Hi Tom,
On Mon, Oct 28, 2013 at 9:32 AM, Tom Rini trini@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-sjg@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.