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

[Simon Glass]

Hi Albert,

On Tue, Nov 8, 2011 at 11:46 AM, Albert ARIBAUD
<albert.u.boot at aribaud.net> wrote:
> Le 08/11/2011 16:57, Simon Glass a écrit :
>>
>> Hi Detlev,
>>
>> On Tue, Nov 8, 2011 at 1:20 AM, Detlev Zundel<dzu at denx.de>  wrote:
>>>
>>> Hi Mike,
>>>
>>>> On Monday 31 October 2011 17:06:46 Simon Glass wrote:
>>>>>
>>>>> On Sun, Oct 30, 2011 at 5:44 PM, Mike Frysinger wrote:
>>>>>>
>>>>>> On Sunday 23 October 2011 23:44:35 Simon Glass wrote:
>>>>>>>
>>>>>>> --- a/arch/arm/lib/board.c
>>>>>>> +++ b/arch/arm/lib/board.c
>>>>>>>
>>>>>>>       flash_size = flash_init();
>>>>>>>       if (flash_size>  0) {
>>>>>>>  # ifdef CONFIG_SYS_FLASH_CHECKSUM
>>>>>>> +             char *s = getenv("flashchecksum");
>>>>>>> +
>>>>>>>               print_size(flash_size, "");
>>>>>>>               /*
>>>>>>>                * Compute and print flash CRC if flashchecksum is set
>>>>>>> to
>>>>>>> 'y' *
>>>>>>>                * NOTE: Maybe we should add some WATCHDOG_RESET()? XXX
>>>>>>>                */
>>>>>>> -             s = getenv("flashchecksum");
>>>>>>>               if (s&&  (*s == 'y')) {
>>>>>>>                       printf("  CRC: %08X", crc32(0,
>>>>>>>                               (const unsigned char *)
>>>>>>> CONFIG_SYS_FLASH_BASE, @@ -566,9 +567,12 @@ void board_init_r(gd_t
>>>>>>> *id,
>>>>>>> ulong dest_addr) /* Initialize from environment */
>>>>>>>       load_addr = getenv_ulong("loadaddr", 16, load_addr);
>>>>>>>  #if defined(CONFIG_CMD_NET)
>>>>>>> -     s = getenv("bootfile");
>>>>>>> -     if (s != NULL)
>>>>>>> -             copy_filename(BootFile, s, sizeof(BootFile));
>>>>>>> +     {
>>>>>>> +             char *s = getenv("bootfile");
>>>>>>> +
>>>>>>> +             if (s != NULL)
>>>>>>> +                     copy_filename(BootFile, s, sizeof(BootFile));
>>>>>>> +     }
>>>>>>>  #endif
>>>>>>
>>>>>> seems like a better solution would be to use at the top:
>>>>>>        __maybe_unused char *s;
>>>>>>
>>>>>> also, shouldn't these be "const char *s" ?
>>>>>
>>>>> We can certainly do this and I agree it is easier than #ifdefs. Does
>>>>> it introduce the possibility that one day the code will stop using the
>>>>> variable but it will still be declared? Is the fact that we need the
>>>>> #ifdefs an indication that the function should be too long and should
>>>>> be refactored? it in fact better to have these explicit so we can see
>>>>> them for the ugliness they are?
>>>>
>>>> yes, you're right that it does leave the door open to the variable being
>>>> declared, never used, and gcc not emitting a warning about it.
>>>>
>>>> both setups suck, but i'd lean towards the less-ifdef state ... wonder
>>>> if
>>>> Wolfgang has a preference.
>>>
>>> Personally, I think that the nuisance of a potential unused variable is
>>> less of an issue than the actual _problems_ that ifdefs induce.
>>
>> Yes the #ifdefs are a pain. I am working on a replacement for board.c
>> - so far I have split things into functions. Next I need to look at
>> Graeme's initcall patch.
>
> I don't think 'ifdefs are' necessarily 'a pain'. They cater for a need, that
> is, to mark that some code depends on some condition. I find it *normal*
> that a checksum-related variable is conditioned on the checksum macro being
> defined.
>

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.

Regards,
Simon

>
> Amicalement,
> --
> Albert.
>