public inbox for u-boot@lists.denx.de
 help / color / mirror / Atom feed
From: Detlev Zundel <dzu@denx.de>
To: u-boot@lists.denx.de
Subject: [U-Boot] [PATCH v2] arm: Correct build error introduced by getenv_ulong() patch
Date: Tue, 08 Nov 2011 10:20:32 +0100	[thread overview]
Message-ID: <m2pqh3dl7j.fsf@ohwell.denx.de> (raw)
In-Reply-To: <201110311740.04957.vapier@gentoo.org> (Mike Frysinger's message of "Mon, 31 Oct 2011 17:40:04 -0400")

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.

Cheers
  Detlev

-- 
The best way to predict the future is to invent it.
                                           -- Alan Kay
--
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

  reply	other threads:[~2011-11-08  9:20 UTC|newest]

Thread overview: 21+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2011-10-24  0:14 [U-Boot] [PATCH] arm: Correct build error introduced by getenv_ulong() patch Simon Glass
2011-10-24  3:44 ` [U-Boot] [PATCH v2] " Simon Glass
2011-10-24 19:13   ` Wolfgang Denk
2011-10-25  6:52     ` Albert ARIBAUD
2011-10-25  7:50       ` Wolfgang Denk
2011-10-25 18:21         ` Albert ARIBAUD
2011-10-25 18:26           ` Simon Glass
2011-10-25 19:36           ` Wolfgang Denk
2011-10-25 19:41             ` Simon Glass
2011-10-31  0:44   ` Mike Frysinger
2011-10-31 21:06     ` Simon Glass
2011-10-31 21:40       ` Mike Frysinger
2011-11-08  9:20         ` Detlev Zundel [this message]
2011-11-08 15:57           ` Simon Glass
2011-11-08 19:46             ` Albert ARIBAUD
2011-11-08 22:28               ` Simon Glass
2011-11-08 22:49                 ` Wolfgang Denk
2011-11-08 23:18                   ` Graeme Russ
2011-11-09 13:45                     ` Detlev Zundel
2011-11-09 14:30                       ` Simon Glass
2011-11-09 14:11                   ` Simon Glass

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=m2pqh3dl7j.fsf@ohwell.denx.de \
    --to=dzu@denx.de \
    --cc=u-boot@lists.denx.de \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox