| Message ID | 1357596628-27501-1-git-send-email-yorksun@freescale.com |
|---|---|
| State | Superseded |
| Headers | show |
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
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.
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
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.
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
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.
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
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.
* 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
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
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
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.
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,
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.
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
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.
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,
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.
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
'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(-)