[U-Boot] [PATCH v2] arm: Correct build error introduced by getenv_ulong() patch

[Simon Glass]

Hi Detlev,

On Wed, Nov 9, 2011 at 5:45 AM, Detlev Zundel <dzu at denx.de> wrote:
> Hi Graeme,
>
>> Hi Wolfgang
>>
>> On Wed, Nov 9, 2011 at 9:49 AM, Wolfgang Denk <wd at denx.de> wrote:
>>> Dear Simon Glass,
>>>
>>> In message <CAPnjgZ15f_gva5+MM1Em-L2sMxt1WAATxqiKUHOQAT893t9MMw at mail.gmail.com> you wrote:
>>>>
>>>> This discussion was regarding the need to #ifdef the variable declaration, viz:
>>>>
>>>> #if defined(THING1) || defined(THING2)
>>>> const char *cat;
>>>> #endif
>>>>
>>>> ...
>>>>
>>>>
>>>> #ifdef THING1
>>>>    cat = getenv("cat");
>>>>
>>>>    send_back(cat);
>>>> #endif
>>>>
>>>> ....
>>>>
>>>> #ifdef THING2
>>>>    cat = check_outside("cat");
>>>>
>>>>    if (cat)
>>>>       wibble(cat);
>>>> #endif
>>>>
>>>>
>>>> and whether the top bit would be better as:
>>>>
>>>> __maybe_unused const char *cat;
>>>>
>>>> But more generally, lots of #ifdefs do make the code harder to read,
>>>> and potentially more brittle in the face of config changes.
>>>
>>> I  would like to see only a minimal number of "__maybe_unused" in the
>>> code - in cases, where this is the way that hurts least.
>>>
>>> In the examples above, it might be better to use local blocks, like:
>>>
>>> #ifdef THING1
>>>        {
>>>                const char *cat = getenv("cat");
>>>
>>>                send_back(cat);
>>>        }
>>> #endif
>>
>> I honestly think most of these cases can be factored out into functions.
>> The compiler should inline them anyway so the overhead should be zero.
>> The various board.c files are a prime example of where this should be
>> done as a matter of principle to reduce the complexity and lenght of
>> the primary function anyway
>
> I would even like to skip the ifdefs completely.  Modern compilers with
> dead code elimination will completely do away unneeded code _but still
> do syntax checks on the parts every time_.

Yes I agree it is better and we are heading that way (e.g. debug
macro). But there are details...

>
> So maybe we should simply try to use
>
>    if (THING1)
>    {
>         ...
>    }
>
> I know that this would need an "#ifdef THING1 1" but errors in this
> would be caught immediately (and not only under a certain combination of
> ifdefs) by the compiler so I don't think this is a problem.

Do you mean '#define THING1 1"?

Can you please restate this without the code inside {} removed? And
let's change the example to CONFIG. Sadly I much prefer #ifdef to #if.

#ifdef CONFIG_THING1
{
    const char *cat;

    cat = getenv("cat");

    send_back(cat);
}
#endif

Things to consider:

- do we include the cat.h header file unconditionally? I assume yes
- iwc does the cat.h header file have #ifdefs to hide its functions /
data structures? If so you get compile errors when you make a mistake;
if not you get link errors which are normally worse
- under what circs. should we define static inline send_back(void) {}
for the case where CONFIG_THING1 is not defined (or is 0)?
- if send_back() is in a C file which is only linked in if THING1 is
defined, does this work as expected?
- CONFIG_THING1 is normally defined but not given a '1' value. Doesn't
this break things?

>
> I don't know how often I repeat my mantra, but every ifdef doubles
> the number of _different source codes_ that we deal with.

Yes of course at the compiler level. Look at all the build
errors/warnings when the new debug() macro came in. But if() creates
different executable code too, and if we resort to hiding lots of
things in static inlines and dead code calls to functions which are
not linked, it can create confusion. It would be good to get some
guidelines on this.

Regards,
Simon

>
> Cheers
>  Detlev
>
> --
> Lotus Notes (GUI):   Run away from it.
>                           -- linux/Documentation/email-clients.txt
> --
> 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
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot
>