diff mbox

[U-Boot] cmd_test: fix a compile error on Blackfin

Message ID 20131119070144.70D40380481@gemini.denx.de
State Accepted
Delegated to: Tom Rini
Headers show

Commit Message

Wolfgang Denk Nov. 19, 2013, 7:01 a.m. UTC
Dear Masahiro Yamada,

In message <1384830117-25345-1-git-send-email-yamada.m@jp.panasonic.com> you wrote:
> Before this commit, common/cmd_test.c defined
> _STDBOOL_H in order to avoid including <stdbool.h>.
> But this work-around is not a good idea.

Actually it is a good idea, as it attempts to be independent of the
actual implementation of the bool data types - it does the same no
matter if "true" and "flase" are members or a union or #define'd
constants.

> --- a/common/cmd_test.c
> +++ b/common/cmd_test.c
> @@ -5,15 +5,6 @@
>   * SPDX-License-Identifier:	GPL-2.0+
>   */
>  
> -/*
> - * Define _STDBOOL_H here to avoid macro expansion of true and false.
> - * If the future code requires macro true or false, remove this define
> - * and undef true and false before U_BOOT_CMD. This define and comment
> - * shall be removed if change to U_BOOT_CMD is made to take string
> - * instead of stringifying it.
> - */
> -#define _STDBOOL_H
> -
>  #include <common.h>
>  #include <command.h>
>  
> @@ -143,6 +134,12 @@ U_BOOT_CMD(
>  	"[args..]"
>  );
>  
> +/*
> + * Undef true and false here to avoid macro expansion by <stdbool.h>
> + */
> +#undef true
> +#undef false
> +

I don't like this.  I feel we should not change global files (that
build fine for everyone else) to work around problems in one specific
implementation.  Instead, we should fix the problem at the root cause,
for example like that.   Could you please test if this patch fixes the
problem, too?


From f68e524dd72c9cc08e86b479b82eff59ef6d591b Mon Sep 17 00:00:00 2001
From: Wolfgang Denk <wd@denx.de>
Date: Tue, 19 Nov 2013 07:50:46 +0100
Subject: [PATCH] Blackfin: don't use 'bool' when it causes problems

The use of 'bool' data types in globally used header files cases build
errors like this:

In file included from arch/blackfin/include/asm/blackfin.h:13:0,
                 from include/common.h:92,
                 from cmd_test.c:17:
arch/blackfin/include/asm/blackfin_local.h:54:1: error: unknown type name 'bool'

Use plain 'int' instead to avoid such kind of trouble.

Signed-off-by: Wolfgang Denk <wd@denx.de>
---
I always had the opinion that the use of "bool" types causes more
problems than it solves.  This case reinforces my opinion. - wd

 arch/blackfin/cpu/os_log.c                 | 6 +++---
 arch/blackfin/include/asm/blackfin_local.h | 2 +-
 2 files changed, 4 insertions(+), 4 deletions(-)

Comments

Masahiro Yamada Nov. 19, 2013, 7:50 a.m. UTC | #1
Hello Wolfgang


> In message <1384830117-25345-1-git-send-email-yamada.m@jp.panasonic.com> you wrote:
> > Before this commit, common/cmd_test.c defined
> > _STDBOOL_H in order to avoid including <stdbool.h>.
> > But this work-around is not a good idea.
> 
> Actually it is a good idea, as it attempts to be independent of the
> actual implementation of the bool data types - it does the same no
> matter if "true" and "flase" are members or a union or #define'd
> constants.

If you think so,  the following also depends on the impilementation
of <stdbool.h>, doesn't it?

#define _STDBOOL_H

#include <common.h>
#include <command.h>


For example, if <stdbool.h> used
_STDBOOL_H_  or  __STDBOOL_H  instead of _STDBOOL_H,
<stdbool.h> would be included and 
common/cmd_test.c  would not be compiled correctly.




> I don't like this.  I feel we should not change global files (that
> build fine for everyone else) to work around problems in one specific
> implementation.  Instead, we should fix the problem at the root cause,
> for example like that.   Could you please test if this patch fixes the
> problem, too?
> 
> 
> From f68e524dd72c9cc08e86b479b82eff59ef6d591b Mon Sep 17 00:00:00 2001
> From: Wolfgang Denk <wd@denx.de>
> Date: Tue, 19 Nov 2013 07:50:46 +0100
> Subject: [PATCH] Blackfin: don't use 'bool' when it causes problems

[snipped]

> @@ -51,7 +51,7 @@ extern u_long get_dclk(void);
>  
>  # define bfin_revid() (bfin_read_CHIPID() >> 28)
>  
> -extern bool bfin_os_log_check(void);
> +extern int bfin_os_log_check(void);
>  extern void bfin_os_log_dump(void);
>  
>  extern void blackfin_icache_flush_range(const void *, const void *);

Yes. Your patch fixed the build error.



Best Regards
Masahiro Yamada
Masahiro Yamada Nov. 25, 2013, 1:43 a.m. UTC | #2
Hello Wolfgang

Could you resend
http://patchwork.ozlabs.org/patch/292309/
in a correct format?

(Or Tom can directly pick it from the ML?)


Best Regards
Masahiro Yamada
Tom Rini Nov. 25, 2013, 1:06 p.m. UTC | #3
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 11/24/2013 08:43 PM, Masahiro Yamada wrote:
> Hello Wolfgang
> 
> Could you resend http://patchwork.ozlabs.org/patch/292309/ in a
> correct format?
> 
> (Or Tom can directly pick it from the ML?)

It's in my current local queue, I'll be pushing things later today.

- -- 
Tom
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iQIcBAEBAgAGBQJSk0tuAAoJENk4IS6UOR1WE3YP/0cE/QOdExTnUwLqtGwoRlIX
bxugoR/7CfHF/o6bvv2Hh9KDXiydSeT7iXt3UauSBbUADC7jjDpBe6YFIteXVeew
xXzqx5fBoyEWHvSD66WyfSha06XQWj8isMn0AO/Tl1AfkYlBNbieSMhE6I1rGiC1
OoCxleTV1Ro5TDmVSf1ev0/K8h7uWXfYHbjX/EGqIoA65+nuUtlASp7Q2IOlZFEe
sTpGK07iPZEouFTt/KZhoV8uw3EERcW7AgaeYUUTYESYpYIAAI4w9PeejY7fOk01
38hXok93kvrLZqQ0HQn2pzVSd/FYCbh6Sx7o/FoTgtlmfHyEzoW/sVYCXj8lP82S
5mlXQwtIEDEFh7cnXYC4crOqyyGjyBrmpIVJ5QXGgS97ze/Vw6u/4iuoqqYg7uL+
voaYMeqi1UUs2dJFA9JO9+9bnzhPNRsf2cBcyf9keXm7GP1+IuoDw9BOX4e81Va0
XYPu8rJZ5GN20k4CAaoDG63PbYN42sR+rbERYJFe1HQ+ECtcuwVHN1xQdCHgOpUV
RYYpyC7WM7OK6TDQjuPAcEmGTioBWL/rz/dniFz1gvsFN0NEVEcPrf9l7JVloyEk
1mPUb4TsuOn0/rmcOXfsVtBmFac12ggy9Ci2xW1W06u9hA0e3EUGzYNxhMQPHoVT
5Y13IpA7dxCDYUHjMo7Q
=4mEj
-----END PGP SIGNATURE-----
Wolfgang Denk Nov. 25, 2013, 2:19 p.m. UTC | #4
Dear Masahiro Yamada,

In message <20131125104322.C02E.AA925319@jp.panasonic.com> you wrote:
> Hello Wolfgang
> 
> Could you resend
> http://patchwork.ozlabs.org/patch/292309/
> in a correct format?
> 
> (Or Tom can directly pick it from the ML?)

Of course I can.  Does this patch solve the problem for you?

Best regards,

Wolfgang Denk
Tom Rini Nov. 25, 2013, 9:58 p.m. UTC | #5
On Tue, Nov 19, 2013 at 08:01:44AM +0100, Wolfgang Denk wrote:
> From: Wolfgang Denk <wd@denx.de>
> Date: Tue, 19 Nov 2013 07:50:46 +0100
> Subject: [PATCH] Blackfin: don't use 'bool' when it causes problems
> 
> The use of 'bool' data types in globally used header files cases build
> errors like this:
> 
> In file included from arch/blackfin/include/asm/blackfin.h:13:0,
>                  from include/common.h:92,
>                  from cmd_test.c:17:
> arch/blackfin/include/asm/blackfin_local.h:54:1: error: unknown type name 'bool'
> 
> Use plain 'int' instead to avoid such kind of trouble.
> 
> Signed-off-by: Wolfgang Denk <wd@denx.de>

Applied to u-boot/master (and re-worded patchwork dl'd patch to match
this), thanks!
Masahiro Yamada Nov. 26, 2013, 10:34 a.m. UTC | #6
Hello Wolfgang


> > Could you resend
> > http://patchwork.ozlabs.org/patch/292309/
> > in a correct format?
> > 
> > (Or Tom can directly pick it from the ML?)
> 
> Of course I can.  Does this patch solve the problem for you?

Yes. It worked for me. Thanks.
(And applied to u-boot/master)

It's OK for now because my motivation is to fix the build error
on blackfin boards.
Although it is true that I am a little concerned about this parts:

#define _STDBOOL_H

#include <common.h>
#include <command.h>


Best Regards
Masahiro Yamada
diff mbox

Patch

diff --git a/arch/blackfin/cpu/os_log.c b/arch/blackfin/cpu/os_log.c
index e1c8e29..2092d9e 100644
--- a/arch/blackfin/cpu/os_log.c
+++ b/arch/blackfin/cpu/os_log.c
@@ -12,12 +12,12 @@ 
 #define OS_LOG_MAGIC_ADDR  ((unsigned long *)0x4f0)
 #define OS_LOG_PTR_ADDR    ((char **)0x4f4)
 
-bool bfin_os_log_check(void)
+int bfin_os_log_check(void)
 {
 	if (*OS_LOG_MAGIC_ADDR != OS_LOG_MAGIC)
-		return false;
+		return 0;
 	*OS_LOG_MAGIC_ADDR = 0;
-	return true;
+	return 1;
 }
 
 void bfin_os_log_dump(void)
diff --git a/arch/blackfin/include/asm/blackfin_local.h b/arch/blackfin/include/asm/blackfin_local.h
index ab31dcb..8ea8cde 100644
--- a/arch/blackfin/include/asm/blackfin_local.h
+++ b/arch/blackfin/include/asm/blackfin_local.h
@@ -51,7 +51,7 @@  extern u_long get_dclk(void);
 
 # define bfin_revid() (bfin_read_CHIPID() >> 28)
 
-extern bool bfin_os_log_check(void);
+extern int bfin_os_log_check(void);
 extern void bfin_os_log_dump(void);
 
 extern void blackfin_icache_flush_range(const void *, const void *);