Patchwork [U-Boot] Introduce a global bool type

login
register
mail settings
Submitter York Sun
Date Jan. 7, 2013, 10:10 p.m.
Message ID <1357596628-27501-1-git-send-email-yorksun@freescale.com>
Download mbox | patch
Permalink /patch/210255/
State Superseded
Headers show

Comments

York Sun - Jan. 7, 2013, 10:10 p.m.
'bool' is defined in random places. This patch consolidates them into a
single typedef.

Signed-off-by: York Sun <yorksun@freescale.com>
---
 arch/blackfin/include/asm/posix_types.h |    3 ---
 board/Marvell/include/core.h            |    5 -----
 drivers/mtd/nand/mxc_nand.c             |    2 --
 drivers/usb/musb-new/linux-compat.h     |    2 --
 include/galileo/core.h                  |    5 -----
 include/linux/types.h                   |    2 ++
 include/xyzModem.h                      |    5 -----
 7 files changed, 2 insertions(+), 22 deletions(-)
Scott Wood - Jan. 7, 2013, 10:29 p.m.
On 01/07/2013 04:10:28 PM, York Sun wrote:
> diff --git a/include/linux/types.h b/include/linux/types.h
> index 1b0b4a4..b359c33 100644
> --- a/include/linux/types.h
> +++ b/include/linux/types.h
> @@ -113,6 +113,8 @@ typedef		__u64		u_int64_t;
>  typedef		__s64		int64_t;
>  #endif
> 
> +typedef _Bool bool;
> +
>  #endif /* __KERNEL_STRICT_NAMES */
> 
>  /*
> diff --git a/include/xyzModem.h b/include/xyzModem.h
> index f437bbd..9723e73 100644
> --- a/include/xyzModem.h
> +++ b/include/xyzModem.h
> @@ -97,11 +97,6 @@ typedef struct {
>  #endif
>  } connection_info_t;
> 
> -#ifndef	BOOL_WAS_DEFINED
> -#define BOOL_WAS_DEFINED
> -typedef unsigned int bool;
> -#endif
> -
>  #define false 0
>  #define true 1

Please also move the definition of true/false into a common header.

-Scott
Timur Tabi - Jan. 7, 2013, 10:32 p.m.
York Sun wrote:
> 'bool' is defined in random places. This patch consolidates them into a
> single typedef.

... and defines 'bool' in a completely different way, so it doesn't just
"consolidate" the definitions.

I would add a comment that says that _Bool was introduced in C99, so it
should be safe to use this new definition instead of a hand-coded enum.
Wolfgang Denk - Jan. 7, 2013, 10:39 p.m.
Dear York Sun,

In message <1357596628-27501-1-git-send-email-yorksun@freescale.com> you wrote:
> 'bool' is defined in random places. This patch consolidates them into a
> single typedef.

Has this been actually compile tested?

...
> --- a/include/linux/types.h
> +++ b/include/linux/types.h
> @@ -113,6 +113,8 @@ typedef		__u64		u_int64_t;
>  typedef		__s64		int64_t;
>  #endif
>  
> +typedef _Bool bool;

And what exactly would "_Bool" be?

...
> --- a/include/xyzModem.h
> +++ b/include/xyzModem.h
> @@ -97,11 +97,6 @@ typedef struct {
>  #endif
>  } connection_info_t;
>  
> -#ifndef	BOOL_WAS_DEFINED
> -#define BOOL_WAS_DEFINED
> -typedef unsigned int bool;
> -#endif
> -
>  #define false 0
>  #define true 1

And don't these remaining definitions of "false" and "true" cause
nasty build errors somewhere?


This seems broken to me.  Can we rather try8 and get rid of all this
"bool" stuff instead?  It's just obfuscating the code...

Best regards,

Wolfgang Denk
Måns Rullgård - Jan. 7, 2013, 10:54 p.m.
Wolfgang Denk <wd@denx.de> writes:

> Dear York Sun,
>
> In message <1357596628-27501-1-git-send-email-yorksun@freescale.com> you wrote:
>> 'bool' is defined in random places. This patch consolidates them into a
>> single typedef.
>
> Has this been actually compile tested?
>
> ...
>> --- a/include/linux/types.h
>> +++ b/include/linux/types.h
>> @@ -113,6 +113,8 @@ typedef		__u64		u_int64_t;
>>  typedef		__s64		int64_t;
>>  #endif
>>  
>> +typedef _Bool bool;
>
> And what exactly would "_Bool" be?

_Bool is a C99 type (though I fail to imagine why).  If using this, one
might as well use the C99 header stdbool.h providing macros for 'bool',
'true' and 'false' instead of this.

> Can we rather try and get rid of all this "bool" stuff instead?  It's
> just obfuscating the code...

Indeed.
Wolfgang Denk - Jan. 8, 2013, 6:25 a.m.
Dear Måns Rullgård,

In message <yw1xpq1g4na0.fsf@unicorn.mansr.com> you wrote:
> 
> >> +typedef _Bool bool;
> >
> > And what exactly would "_Bool" be?
> 
> _Bool is a C99 type (though I fail to imagine why).  If using this, one
> might as well use the C99 header stdbool.h providing macros for 'bool',
> 'true' and 'false' instead of this.

Agreed - if we should really stick with that, that that's the way to
go.

> > Can we rather try and get rid of all this "bool" stuff instead?  It's
> > just obfuscating the code...
> 
> Indeed.

Thanks.


Wolfgang Denk
Tabi Timur-B04825 - Jan. 8, 2013, 4:51 p.m.
On Mon, Jan 7, 2013 at 4:39 PM, Wolfgang Denk <wd@denx.de> wrote:
>
> This seems broken to me.  Can we rather try8 and get rid of all this
> "bool" stuff instead?  It's just obfuscating the code...

Like Scott said, we sometimes copy code from Linux that uses 'bool',
so it's simpler if we just retain this commonly-used type.  If it's
part of the language, how is it obfuscating?  Maybe the Linux
developers should have used _Bool instead of bool, but they didn't,
and so here we are.
Wolfgang Denk - Jan. 8, 2013, 5:49 p.m.
Dear Tabi Timur-B04825,

In message <6AE080B68D46FC4BA2D2769E68D765B70820541F@039-SN2MPN1-023.039d.mgd.msft.net> you wrote:
> >
> > This seems broken to me.  Can we rather try8 and get rid of all this
> > "bool" stuff instead?  It's just obfuscating the code...
> 
> Like Scott said, we sometimes copy code from Linux that uses 'bool',
> so it's simpler if we just retain this commonly-used type.  If it's
> part of the language, how is it obfuscating?  Maybe the Linux

_Bool has been introduced very late to any C standard, and you can
still see this from the ugly, unnatural name.

It is my personal firm conviction that the people pushed it were not
the ones who have been using C right from the beginning, say from the
times of Unix v6 or so.

IMHO it is much better to rely on '0' meaning "false" and anything
else meaning "true" instead of insisting on one specific value of
"true".  Yes, people claim the code is easier to read and understand,
but these are the same people who claim drop-down menues are easier to
work wit than a CLI.  And I've seen more than one case where bugs were
caused by using "proper bool types" like this:

	i = 0;
	j = 0;
	k = 2;

	if ((i | j | k) == true) ...


> developers should have used _Bool instead of bool, but they didn't,
> and so here we are.

Well, I raised my concerns, but I do not intend to formally NAK it.
In any case, I insist on using the standard header file.

Best regards,

Wolfgang Denk
Tabi Timur-B04825 - Jan. 8, 2013, 5:53 p.m.
Wolfgang Denk wrote:
> Dear Tabi Timur-B04825,
> 
> In message <6AE080B68D46FC4BA2D2769E68D765B70820541F@039-SN2MPN1-023.039d.mgd.msft.net> you wrote:
>>>
>>> This seems broken to me.  Can we rather try8 and get rid of all this
>>> "bool" stuff instead?  It's just obfuscating the code...

>>
>> Like Scott said, we sometimes copy code from Linux that uses 'bool',
>> so it's simpler if we just retain this commonly-used type.  If it's
>> part of the language, how is it obfuscating?  Maybe the Linux
> 
> _Bool has been introduced very late to any C standard, and you can
> still see this from the ugly, unnatural name.

It was introduced in C99, which is over 12 years old.

> It is my personal firm conviction that the people pushed it were not
> the ones who have been using C right from the beginning, say from the
> times of Unix v6 or so.
> 
> IMHO it is much better to rely on '0' meaning "false" and anything
> else meaning "true" instead of insisting on one specific value of
> "true".  Yes, people claim the code is easier to read and understand,
> but these are the same people who claim drop-down menues are easier to
> work wit than a CLI.  And I've seen more than one case where bugs were
> caused by using "proper bool types" like this:
> 
> 	i = 0;
> 	j = 0;
> 	k = 2;
> 
> 	if ((i | j | k) == true) ...

Ok, but this is just wrong.  i, j, and k are not boolean types, so they
should not be compared with 'true' or 'false'.  I don't think you'll find
any disagreement with that.
Bernhard Walle - Jan. 8, 2013, 6:34 p.m.
* Wolfgang Denk <wd@denx.de> [2013-01-08 18:49]:
> In message <6AE080B68D46FC4BA2D2769E68D765B70820541F@039-SN2MPN1-023.039d.mgd.msft.net> you wrote:
> > >
> > > This seems broken to me.  Can we rather try8 and get rid of all this
> > > "bool" stuff instead?  It's just obfuscating the code...
> > 
> > Like Scott said, we sometimes copy code from Linux that uses 'bool',
> > so it's simpler if we just retain this commonly-used type.  If it's
> > part of the language, how is it obfuscating?  Maybe the Linux
> 
> _Bool has been introduced very late to any C standard, and you can
> still see this from the ugly, unnatural name.

But C99 (well, that's 12 years now!) also includes <stdbool.h> that
defines 'bool', 'true' and 'false'.


Regards,
Bernhard
Wolfgang Denk - Jan. 8, 2013, 7:07 p.m.
Dear Timur Tabi,

In message <50EC5D29.1070408@freescale.com> you wrote:
>
> > _Bool has been introduced very late to any C standard, and you can
> > still see this from the ugly, unnatural name.
> 
> It was introduced in C99, which is over 12 years old.

And how old is C?   I think the "official" announcment was 1972, so
that's more than twice as long without that addition.

> > work wit than a CLI.  And I've seen more than one case where bugs were
> > caused by using "proper bool types" like this:
> > 
> > 	i = 0;
> > 	j = 0;
> > 	k = 2;
> > 
> > 	if ((i | j | k) == true) ...
> 
> Ok, but this is just wrong.  i, j, and k are not boolean types, so they
> should not be compared with 'true' or 'false'.  I don't think you'll find
> any disagreement with that.

You are right.  And I wrote that it's a bug.  But this is what you can
easily get from using boolean types.  This is example has not been
invented by me.  I don't even claim that this was good programming
style - all I want to say is that from what I have seen the boolean
types are not a panacea; they cause new problems as well.

Best regards,

Wolfgang Denk
Wolfgang Denk - Jan. 8, 2013, 7:08 p.m.
Dear Bernhard Walle,

In message <20130108183424.GA2761@regiomontanus.your-server.de> you wrote:
>
> > _Bool has been introduced very late to any C standard, and you can
> > still see this from the ugly, unnatural name.
> 
> But C99 (well, that's 12 years now!) also includes <stdbool.h> that
> defines 'bool', 'true' and 'false'.

That's strange - you make the same mistake as Timur.

For me 2013 - 1999 != 12 :-P

Best regards,

Wolfgang Denk
Tabi Timur-B04825 - Jan. 8, 2013, 7:09 p.m.
Wolfgang Denk wrote:
> You are right.  And I wrote that it's a bug.  But this is what you can
> easily get from using boolean types.  This is example has not been
> invented by me.  I don't even claim that this was good programming
> style - all I want to say is that from what I have seen the boolean
> types are not a panacea; they cause new problems as well.

I don't disagree with any of that, but I don't see what your point is.
Every time you use a new feature, there are also new ways to use it
incorrectly.  By your logic, we should use no new features of the C
language that were invented in the past 20 years.
Albert ARIBAUD - Jan. 19, 2013, 9:30 a.m.
Hi Wolfgang,

My 2 EUR cents:

On Tue, 08 Jan 2013 20:07:15 +0100, Wolfgang Denk <wd@denx.de> wrote:

(sorry for the late chiming in)

> Dear Timur Tabi,
> 
> In message <50EC5D29.1070408@freescale.com> you wrote:
> >
> > > _Bool has been introduced very late to any C standard, and you can
> > > still see this from the ugly, unnatural name.
> > 
> > It was introduced in C99, which is over 12 years old.
> 
> And how old is C?   I think the "official" announcment was 1972, so
> that's more than twice as long without that addition.
> 
> > > work wit than a CLI.  And I've seen more than one case where bugs were
> > > caused by using "proper bool types" like this:
> > > 
> > > 	i = 0;
> > > 	j = 0;
> > > 	k = 2;
> > > 
> > > 	if ((i | j | k) == true) ...
> 
> > Ok, but this is just wrong.  i, j, and k are not boolean types, so they
> > should not be compared with 'true' or 'false'.  I don't think you'll find
> > any disagreement with that.
> 
> You are right.  And I wrote that it's a bug.  But this is what you can
> easily get from using boolean types.  This is example has not been
> invented by me.  I don't even claim that this was good programming
> style - all I want to say is that from what I have seen the boolean
> types are not a panacea; they cause new problems as well.

Ok, so there are three things in Wolfgang's example: a lax boolean (set
to 2), a mix-up between bitwise and boolean operators (which a compiler
may or may not detect or at least flag as suspicious), and finally a
comparison of the lax boolean (2) with a strict boolean (true, equal
to 1) which will fail.

I guess we're all aware of this type of problem. To avoid it, I
personally try to apply the Postel principle here: be conservative in
what you do, thus only produce strict boolean objects, and be liberal
in what you get, i.e. consider all boolean expressions to be lax.

This means that as far as coding practice is concerned, I tend to favor
the style set forth in the next three lines, where I always compute 
booleans with true and false and boolean operators, but test them
'zero/nonzero':

	/* what I favor */
	clk_is_enabled = ((reg_val >> 9) & 1) ? true: false;
	ip_is_enabled = clk_is_enabled && pwd_is_enabled;
	if (clk_is_enabled) { ...

rather than assigning them 'zero/nonzero', or using bitwise ops on
booleans, or testing against boolean constants (although I concede
that the first line below wins over its counterpart above as far as
concision is concerned).

	/* what I don't favor */
	clk_is_enabled = ((register >> 9) & 1);
	ip_is_enabled = clk_is_enabled & pwd_is_enabled;
	if (clk_is_enabled == true) { ...

This way I am sure to evaluate any nonzero value as 'true', so lax code
can safely pass me lax booleans, and I am sure that my booleans equal 1
when true, so I always pass strict booleans to strict code.

Oh, and I also try to wisely name boolean objects, so that they read out
loud as a boolean statement, e.g. "clock is enabled", but this is a bit
(more) beside the point.

> Best regards,
> 
> Wolfgang Denk

Amicalement,
Måns Rullgård - Jan. 21, 2013, 10:36 p.m.
Scott Wood <scottwood@freescale.com> writes:

> On 01/19/2013 03:30:30 AM, Albert ARIBAUD wrote:
>> 	/* what I favor */
>> 	clk_is_enabled = ((reg_val >> 9) & 1) ? true: false;
>> 	ip_is_enabled = clk_is_enabled && pwd_is_enabled;
>> 	if (clk_is_enabled) { ...
>> 
>> rather than assigning them 'zero/nonzero', or using bitwise ops on
>> booleans, or testing against boolean constants (although I concede
>> that the first line below wins over its counterpart above as far as
>> concision is concerned).
>
> Conciseness can be improved with "!!((reg_val >> 9) & 1)".

x & 1 is already either zero or one.  Any further operations are nothing
but obfuscation.
Scott Wood - Jan. 21, 2013, 10:51 p.m.
On 01/21/2013 04:36:42 PM, Måns Rullgård wrote:
> Scott Wood <scottwood@freescale.com> writes:
> 
> > On 01/19/2013 03:30:30 AM, Albert ARIBAUD wrote:
> >> 	/* what I favor */
> >> 	clk_is_enabled = ((reg_val >> 9) & 1) ? true: false;
> >> 	ip_is_enabled = clk_is_enabled && pwd_is_enabled;
> >> 	if (clk_is_enabled) { ...
> >>
> >> rather than assigning them 'zero/nonzero', or using bitwise ops on
> >> booleans, or testing against boolean constants (although I concede
> >> that the first line below wins over its counterpart above as far as
> >> concision is concerned).
> >
> > Conciseness can be improved with "!!((reg_val >> 9) & 1)".
> 
> x & 1 is already either zero or one.  Any further operations are  
> nothing
> but obfuscation.

The point is to avoid depending on the actual integer values of the  
boolean type, and make the code more robust against changes (e.g.  
someone later comes along and says "hmm, that 1 should be a 3 because  
we care about that other register bit as well" without noticing that  
it's being assigned to a boolean.

-Scott
Måns Rullgård - Jan. 21, 2013, 11:08 p.m.
Scott Wood <scottwood@freescale.com> writes:

> On 01/21/2013 04:36:42 PM, Måns Rullgård wrote:
>> Scott Wood <scottwood@freescale.com> writes:
>> 
>> > On 01/19/2013 03:30:30 AM, Albert ARIBAUD wrote:
>> >> 	/* what I favor */
>> >> 	clk_is_enabled = ((reg_val >> 9) & 1) ? true: false;
>> >> 	ip_is_enabled = clk_is_enabled && pwd_is_enabled;
>> >> 	if (clk_is_enabled) { ...
>> >>
>> >> rather than assigning them 'zero/nonzero', or using bitwise ops on
>> >> booleans, or testing against boolean constants (although I concede
>> >> that the first line below wins over its counterpart above as far as
>> >> concision is concerned).
>> >
>> > Conciseness can be improved with "!!((reg_val >> 9) & 1)".
>> 
>> x & 1 is already either zero or one.  Any further operations are  
>> nothing
>> but obfuscation.
>
> The point is to avoid depending on the actual integer values of the  
> boolean type,

Boolean expressions are defined to have a value of zero or one, and the
_Bool type (may it burn in hell) must be able to represent those values.

> and make the code more robust against changes (e.g. someone later
> comes along and says "hmm, that 1 should be a 3 because we care about
> that other register bit as well" without noticing that it's being
> assigned to a boolean.

If you stayed away from the silly _Bool type that wouldn't be a problem,
as long as any uses of the value treat all non-zero values equally, i.e. 
only use it in a boolean context and not compare against explicit values
or perform arithmetic or bitwise logic operations on it.

In other words, boolifying on use rather than on assignment is generally
safer and usually at least as efficient.
Albert ARIBAUD - Jan. 22, 2013, 7:41 a.m.
Hi Måns,

> In other words, boolifying on use rather than on assignment is generally
> safer and usually at least as efficient.

Except when assigning a C = A & B where A and B happen to have
no common bit set. Which is why I think 'boolifying' as soon as
possible -- on assignment -- is way safer than on use.

Amicalement,
Måns Rullgård - Jan. 22, 2013, 12:59 p.m.
Albert ARIBAUD <albert.u.boot@aribaud.net> writes:

> Hi Måns,
>
>> In other words, boolifying on use rather than on assignment is generally
>> safer and usually at least as efficient.
>
> Except when assigning a C = A & B where A and B happen to have
> no common bit set. Which is why I think 'boolifying' as soon as
> possible -- on assignment -- is way safer than on use.

But that's not a boolean context.  You should use A && B.

The thing is, when using a value, you know if you need a boolean, but
not necessarily (easily) how the value was assigned.  Conversely, when
assigning a value, you do not know how it will be used.  By always
boolifying on use, you remove the need to keep track of which values are
"true" booleans and which ones are arbitrary values (or even pointers)
you happen to be using in a boolean fashion.

Patch

diff --git a/arch/blackfin/include/asm/posix_types.h b/arch/blackfin/include/asm/posix_types.h
index 000ffe5..1f28b36 100644
--- a/arch/blackfin/include/asm/posix_types.h
+++ b/arch/blackfin/include/asm/posix_types.h
@@ -61,9 +61,6 @@  typedef unsigned int __kernel_gid32_t;
 typedef unsigned short __kernel_old_uid_t;
 typedef unsigned short __kernel_old_gid_t;
 
-#define BOOL_WAS_DEFINED
-typedef enum { false = 0, true = 1 } bool;
-
 #ifdef __GNUC__
 typedef long long __kernel_loff_t;
 #endif
diff --git a/board/Marvell/include/core.h b/board/Marvell/include/core.h
index c413439..3119d0a 100644
--- a/board/Marvell/include/core.h
+++ b/board/Marvell/include/core.h
@@ -91,11 +91,6 @@  extern unsigned int INTERNAL_REG_BASE_ADDR;
 #define _1G		0x40000000
 #define _2G		0x80000000
 
-#ifndef	BOOL_WAS_DEFINED
-#define BOOL_WAS_DEFINED
-typedef enum _bool{false,true} bool;
-#endif
-
 /* Little to Big endian conversion macros */
 
 #ifdef LE /* Little Endian */
diff --git a/drivers/mtd/nand/mxc_nand.c b/drivers/mtd/nand/mxc_nand.c
index d0ded48..04836c0 100644
--- a/drivers/mtd/nand/mxc_nand.c
+++ b/drivers/mtd/nand/mxc_nand.c
@@ -29,8 +29,6 @@ 
 
 #define DRIVER_NAME "mxc_nand"
 
-typedef enum {false, true} bool;
-
 struct mxc_nand_host {
 	struct mtd_info			mtd;
 	struct nand_chip		*nand;
diff --git a/drivers/usb/musb-new/linux-compat.h b/drivers/usb/musb-new/linux-compat.h
index 5c126ef..72c8c2b 100644
--- a/drivers/usb/musb-new/linux-compat.h
+++ b/drivers/usb/musb-new/linux-compat.h
@@ -12,8 +12,6 @@ 
 #define __iomem
 #define __deprecated
 
-typedef enum { false = 0, true = 1 } bool;
-
 struct unused {};
 typedef struct unused unused_t;
 
diff --git a/include/galileo/core.h b/include/galileo/core.h
index c277509..faf4962 100644
--- a/include/galileo/core.h
+++ b/include/galileo/core.h
@@ -110,11 +110,6 @@  extern unsigned int INTERNAL_REG_BASE_ADDR;
 #define _1G             0x40000000
 #define _2G             0x80000000
 
-#ifndef	BOOL_WAS_DEFINED
-#define BOOL_WAS_DEFINED
-typedef enum _bool{false,true} bool;
-#endif
-
 /* Little to Big endian conversion macros */
 
 #ifdef LE /* Little Endian */
diff --git a/include/linux/types.h b/include/linux/types.h
index 1b0b4a4..b359c33 100644
--- a/include/linux/types.h
+++ b/include/linux/types.h
@@ -113,6 +113,8 @@  typedef		__u64		u_int64_t;
 typedef		__s64		int64_t;
 #endif
 
+typedef _Bool bool;
+
 #endif /* __KERNEL_STRICT_NAMES */
 
 /*
diff --git a/include/xyzModem.h b/include/xyzModem.h
index f437bbd..9723e73 100644
--- a/include/xyzModem.h
+++ b/include/xyzModem.h
@@ -97,11 +97,6 @@  typedef struct {
 #endif
 } connection_info_t;
 
-#ifndef	BOOL_WAS_DEFINED
-#define BOOL_WAS_DEFINED
-typedef unsigned int bool;
-#endif
-
 #define false 0
 #define true 1