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

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

[Simon Glass]

Commit dc8bbea removed a local variable that is used in most ARM boards.

Since we want to avoid an 'unused variable' warning with later compilers,
and the #ifdef logic of whether this variable is required is bit painful,
this declares the variable local to the block of code that needs it.

Signed-off-by: Simon Glass <sjg@chromium.org>
---
 arch/arm/lib/board.c |   12 +++++++++---
 1 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/arch/arm/lib/board.c b/arch/arm/lib/board.c
index c764844..3c147d1 100644
--- a/arch/arm/lib/board.c
+++ b/arch/arm/lib/board.c
@@ -477,6 +477,8 @@ void board_init_r(gd_t *id, ulong dest_addr)
 	flash_size = flash_init();
 	if (flash_size > 0) {
 # ifdef CONFIG_SYS_FLASH_CHECKSUM
+		char *s;
+
 		print_size(flash_size, "");
 		/*
 		 * Compute and print flash CRC if flashchecksum is set to 'y'
@@ -566,9 +568,13 @@ 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;
+
+		s = getenv("bootfile");
+		if (s != NULL)
+			copy_filename(BootFile, s, sizeof(BootFile));
+	}
 #endif
 
 #ifdef BOARD_LATE_INIT
-- 
1.7.3.1

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

[Simon Glass]

Commit dc8bbea removed a local variable that is used in most ARM boards.

Since we want to avoid an 'unused variable' warning with later compilers,
and the #ifdef logic of whether this variable is required is bit painful,
this declares the variable local to the block of code that needs it.

Signed-off-by: Simon Glass <sjg@chromium.org>
---
Changes in v2:
- Tidy up to call getenv() in declaration

 arch/arm/lib/board.c |   12 ++++++++----
 1 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/arch/arm/lib/board.c b/arch/arm/lib/board.c
index c764844..7434b34 100644
--- a/arch/arm/lib/board.c
+++ b/arch/arm/lib/board.c
@@ -477,13 +477,14 @@ void board_init_r(gd_t *id, ulong dest_addr)
 	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
 
 #ifdef BOARD_LATE_INIT
-- 
1.7.3.1

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

[Wolfgang Denk]

Dear Simon Glass,

In message <1319427875-29965-1-git-send-email-sjg@chromium.org> you wrote:
> Commit dc8bbea removed a local variable that is used in most ARM boards.
> 
> Since we want to avoid an 'unused variable' warning with later compilers,
> and the #ifdef logic of whether this variable is required is bit painful,
> this declares the variable local to the block of code that needs it.
> 
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
> Changes in v2:
> - Tidy up to call getenv() in declaration
> 
>  arch/arm/lib/board.c |   12 ++++++++----
>  1 files changed, 8 insertions(+), 4 deletions(-)

Applied, thanks.

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: wd at denx.de
The joys of love made her human and the  agonies  of  love  destroyed
her.
	-- Spock, "Requiem for Methuselah", stardate 5842.8

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

[Albert ARIBAUD]

Hi Wolfgang,

Le 24/10/2011 21:13, Wolfgang Denk a ?crit :
> Dear Simon Glass,
>
> In message<1319427875-29965-1-git-send-email-sjg@chromium.org>  you wrote:
>> Commit dc8bbea removed a local variable that is used in most ARM boards.
>>
>> Since we want to avoid an 'unused variable' warning with later compilers,
>> and the #ifdef logic of whether this variable is required is bit painful,
>> this declares the variable local to the block of code that needs it.
>>
>> Signed-off-by: Simon Glass<sjg@chromium.org>
>> ---
>> Changes in v2:
>> - Tidy up to call getenv() in declaration
>>
>>   arch/arm/lib/board.c |   12 ++++++++----
>>   1 files changed, 8 insertions(+), 4 deletions(-)
>
> Applied, thanks.
>
> Best regards,
>
> Wolfgang Denk

I've just fetched u-boot and I don't see this one. Can you push 
u-boot/master so that I can rebase u-boot-arm/master on it and launch 
some tests?

Amicalement,
-- 
Albert.

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

[Wolfgang Denk]

Dear Albert ARIBAUD,

In message <4EA65CBF.7060704@aribaud.net> you wrote:
> 
> I've just fetched u-boot and I don't see this one. Can you push 
> u-boot/master so that I can rebase u-boot-arm/master on it and launch 
> some tests?

Done.

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: wd at denx.de
To get something done, a committee should consist  of  no  more  than
three men, two of them absent.

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

[Albert ARIBAUD]

Le 25/10/2011 09:50, Wolfgang Denk a ?crit :
> Dear Albert ARIBAUD,
>
> In message<4EA65CBF.7060704@aribaud.net>  you wrote:
>>
>> I've just fetched u-boot and I don't see this one. Can you push
>> u-boot/master so that I can rebase u-boot-arm/master on it and launch
>> some tests?
>
> Done.

Hmm... Weird. I've just fetched u-Boot again, and I cannot find this 
patch in there ; the last commit of u-boot/master seems to be
4962e38e9a4a053792722918bb11c5408549aebd, dated 2011-10-17.

Can somebody aside from me check this?

> Best regards,
>
> Wolfgang Denk

Amicalement,
-- 
Albert.

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

[Simon Glass]

Hi Albert,

On Tue, Oct 25, 2011 at 11:21 AM, Albert ARIBAUD
<albert.u.boot@aribaud.net> wrote:
> Le 25/10/2011 09:50, Wolfgang Denk a ?crit :
>>
>> Dear Albert ARIBAUD,
>>
>> In message<4EA65CBF.7060704@aribaud.net> ?you wrote:
>>>
>>> I've just fetched u-boot and I don't see this one. Can you push
>>> u-boot/master so that I can rebase u-boot-arm/master on it and launch
>>> some tests?
>>
>> Done.
>
> Hmm... Weird. I've just fetched u-Boot again, and I cannot find this patch
> in there ; the last commit of u-boot/master seems to be
> 4962e38e9a4a053792722918bb11c5408549aebd, dated 2011-10-17.
>
> Can somebody aside from me check this?

Yes I see the same.

Regards,
Simon

>
>> Best regards,
>>
>> Wolfgang Denk
>
> Amicalement,
> --
> Albert.
>

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

[Wolfgang Denk]

Dear Albert ARIBAUD,

In message <4EA6FE3B.9080207@aribaud.net> you wrote:
>
> >> I've just fetched u-boot and I don't see this one. Can you push
> >> u-boot/master so that I can rebase u-boot-arm/master on it and launch
> >> some tests?
> >
> > Done.
> 
> Hmm... Weird. I've just fetched u-Boot again, and I cannot find this 
> patch in there ; the last commit of u-boot/master seems to be
> 4962e38e9a4a053792722918bb11c5408549aebd, dated 2011-10-17.
> 
> Can somebody aside from me check this?

Sorry, my fault.  Should be fixed now.

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: wd at denx.de
If you're not part of the solution, you're part of the problem.

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

[Simon Glass]

On Tue, Oct 25, 2011 at 12:36 PM, Wolfgang Denk <wd@denx.de> wrote:
> Dear Albert ARIBAUD,
>
> In message <4EA6FE3B.9080207@aribaud.net> you wrote:
>>
>> >> I've just fetched u-boot and I don't see this one. Can you push
>> >> u-boot/master so that I can rebase u-boot-arm/master on it and launch
>> >> some tests?
>> >
>> > Done.
>>
>> Hmm... Weird. I've just fetched u-Boot again, and I cannot find this
>> patch in there ; the last commit of u-boot/master seems to be
>> 4962e38e9a4a053792722918bb11c5408549aebd, dated 2011-10-17.
>>
>> Can somebody aside from me check this?
>
> Sorry, my fault. ?Should be fixed now.

Yes it's fine now, thanks

- Simon

>
> 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: wd at denx.de
> If you're not part of the solution, you're part of the problem.
>

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

[Mike Frysinger]

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" ?
-mike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
Url : http://lists.denx.de/pipermail/u-boot/attachments/20111030/d0f8df3d/attachment.pgp 

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

[Simon Glass]

Hi Mike,

On Sun, Oct 30, 2011 at 5:44 PM, Mike Frysinger <vapier@gentoo.org> 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" ?
> -mike
>

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?

I'm not sure, but thought I should ask.

Regards,
Simon

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

[Mike Frysinger]

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.
-mike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
Url : http://lists.denx.de/pipermail/u-boot/attachments/20111031/948e5d8d/attachment.pgp 

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

[Detlev Zundel]

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

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

[Simon Glass]

Hi Detlev,

On Tue, Nov 8, 2011 at 1:20 AM, Detlev Zundel <dzu@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.
..
Regards,
Simon

>
> 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
>

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

[Albert ARIBAUD]

Le 08/11/2011 16:57, Simon Glass a ?crit :
> Hi Detlev,
>
> On Tue, Nov 8, 2011 at 1:20 AM, Detlev Zundel<dzu@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.

> Regards,
> Simon

Amicalement,
-- 
Albert.

[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@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@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.
>

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

[Wolfgang Denk]

Dear Simon Glass,

In message <CAPnjgZ15f_gva5+MM1Em-L2sMxt1WAATxqiKUHOQAT893t9MMw@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


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: wd at denx.de
"There is such a fine line between genius and stupidity."
- David St. Hubbins, "Spinal Tap"

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

[Graeme Russ]

Hi Wolfgang

On Wed, Nov 9, 2011 at 9:49 AM, Wolfgang Denk <wd@denx.de> wrote:
> Dear Simon Glass,
>
> In message <CAPnjgZ15f_gva5+MM1Em-L2sMxt1WAATxqiKUHOQAT893t9MMw@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

Regards,

Graeme

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

[Detlev Zundel]

Hi Graeme,

> Hi Wolfgang
>
> On Wed, Nov 9, 2011 at 9:49 AM, Wolfgang Denk <wd@denx.de> wrote:
>> Dear Simon Glass,
>>
>> In message <CAPnjgZ15f_gva5+MM1Em-L2sMxt1WAATxqiKUHOQAT893t9MMw@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_.

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.

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

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] [PATCH v2] arm: Correct build error introduced by getenv_ulong() patch

[Simon Glass]

Hi Wolfgang,

On Tue, Nov 8, 2011 at 2:49 PM, Wolfgang Denk <wd@denx.de> wrote:
> Dear Simon Glass,
>
> In message <CAPnjgZ15f_gva5+MM1Em-L2sMxt1WAATxqiKUHOQAT893t9MMw@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
>

Noted, thanks.

Regards,
Simon

>
> 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: wd at denx.de
> "There is such a fine line between genius and stupidity."
> - David St. Hubbins, "Spinal Tap"
>

[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@denx.de> wrote:
> Hi Graeme,
>
>> Hi Wolfgang
>>
>> On Wed, Nov 9, 2011 at 9:49 AM, Wolfgang Denk <wd@denx.de> wrote:
>>> Dear Simon Glass,
>>>
>>> In message <CAPnjgZ15f_gva5+MM1Em-L2sMxt1WAATxqiKUHOQAT893t9MMw@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
>