Re: [PATCH] drivers/scsi/aic7xxx_old: Convert to generic boolean-values
On Fri, 2007-02-16 at 10:50 -0800, Andrew Morton wrote: > Me no understand. > > If you take the specific example of > > void > ahd_set_syncrate(struct ahd_softc *ahd, struct ahd_devinfo *devinfo, >u_int period, u_int offset, u_int ppr_options, >u_int type, int paused) > > then if is crufty, inappropriate and wrong that `paused' is a scalar type. Although you picked a code segment out of the modern aic7xxx, there is a matching similar one in aic7xxx_old. Now, in all fairness, I was at one point playing with a much more preemptable model for that driver that allowed nested pauses, at which point the value of pause would have made sense to be scalar, but that was a *long* time ago. > > It's just not true or sensible that the code is written so that `paused' > can take a value of seventy eight. It _is_ a boolean. It is a truth > value. Declaring it as such in the source is all goodness. Passing the > value `true' into calls to this function improve readability over passing > "1". Hence the reason for the original upper case TRUE/FALSE. I have to admit, I don't really like the lower case true/false, it looks like a variable that can be assigned, thereby changing the implementation of the function call when in fact each calling location is hard coding a constant. But, that's just me and my crufty old C that differentiates between hard coded things and variables via case. > So I don't agree with (or understand) your objections. But I can certainly > understand reluctance to merge a large-but-minor, do-nothing-much patch into > a large and not-very-maintained driver. Hehehe...and here I was thinking of factoring that thing into files and actually bringing it into the current century. -- Doug Ledford <[EMAIL PROTECTED]> GPG KeyID: CFBFF194 http://people.redhat.com/dledford Infiniband specific RPMs available at http://people.redhat.com/dledford/Infiniband signature.asc Description: This is a digitally signed message part
Re: [PATCH] drivers/scsi/aic7xxx_old: Convert to generic boolean-values
James Bottomley wrote: On Fri, 2007-02-16 at 19:04 +0100, Richard Knutsson wrote: James Bottomley wrote: On Mon, 2007-02-12 at 12:27 -0800, Andrew Morton wrote: Given that we now have a standard kernel-wide, c99-friendly way of expressing true and false, I'd suggest that this decision can be revisited. Because a "true" is significantly more meaningful (and hence readable) thing than a bare "1". OK, I'm really not happy with doing this for three reasons: 1. It's inviting huge amounts of driver churn changing bitfields to booleans Have been some work done already. Has there been any problems? There's always an issue when two people work on the same driver ... it causes patch conflicts, which is why we try to avoid it where we can. But it is (hopefully) a one-time change. 2. I do find it to be a readability issue. Like most driver writers, I'm used to register layouts, and those are simple bitfields, so I don't tend to think true and false, I think 1 and 0. It is a fundamental difference between an integer and a boolean. Have you seen anyone trying to do "bool var = true + true;"? ;) I don't quite see how this is relevant to the readability issue? Your "simple bitfields" :) But on the note of readability; there is in no way any constraints to only use 'false'/'true'. Right now I'm converting some files who already uses FALSE/TRUE and may change 0/1 for consitency. But I think it all boils down to what people are used to (I don't think there are any C++/Java-developers who complains about boolean and false/true). I am not too concerned about 0/1 being used or false/true, but the proper use of booleans and values. 3. Having a different, special, type for single bit bitfields (while still using u for multi bit bitfields) is asking for confusion, and hence trouble at the driver level. I don't think a boolean should be view as a single bit bitfield. Ex: u8 a:1; ... int b = 4 + a; is obviously not a boolean, while: u8 a:1; ... if (a) is, and a should be "bool a:1;" (imho) This again, doesn't really address the argument. I'm saying I'd rather not have confusion over what types to use in the driver. You're saying that if you only check the value for truth or falsehood it should be a boolean. That's actually worse than I was anticipating because you're now saying that single bit bitfields may or may not be booleans depending on use. This looks like worse potential for confusion than before. Actually I find it to be simpler. Which would you chose?: u8 a:1; or int a:1; for a value? But if 'a' is a statement, then we just use: bool a:1 (if the size of the variable is of relevance, otherwise just "bool a"). (You know what the variables are going to be used for when declaring them, right??) Btw, how do you know if "a += 1" is just used to flip 'a's value or if it is a bug (in the case of boolean)? Richard "come over to the dark side" Knutsson - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] drivers/scsi/aic7xxx_old: Convert to generic boolean-values
On Fri, 16 Feb 2007 12:42:27 -0600 James Bottomley <[EMAIL PROTECTED]> wrote: > On Fri, 2007-02-16 at 10:34 -0800, Andrew Morton wrote: > > On Fri, 16 Feb 2007 10:42:12 -0600 James Bottomley <[EMAIL PROTECTED]> > > wrote: > > > > > On Mon, 2007-02-12 at 12:27 -0800, Andrew Morton wrote: > > > > Given that we now have a standard kernel-wide, c99-friendly way of > > > > expressing true and false, I'd suggest that this decision can be > > > > revisited. > > > > > > > > Because a "true" is significantly more meaningful (and hence readable) > > > > thing than a bare "1". > > > > > > OK, I'm really not happy with doing this for three reasons: > > > > > > 1. It's inviting huge amounts of driver churn changing bitfields to > > > booleans > > > > > > 2. I do find it to be a readability issue. Like most driver writers, > > > I'm used to register layouts, and those are simple bitfields, so I don't > > > tend to think true and false, I think 1 and 0. > > > > > > 3. Having a different, special, type for single bit bitfields (while > > > still using u for multi bit bitfields) is asking for confusion, and > > > hence trouble at the driver level. > > > > > > > Confused. The patch changes TRUE to true and FALSE to false. The code > > wasn't using bitfields before and isn't using them afterwards. I wouldn't > > expect there to be any change in generated code. > > Sorry, I was addressing the general idea of using booleans in drivers. > > > All it's doing is replacing the driver's private TRUE/FALSE with the > > kernel-wide ones. > > I already addressed that one ... I prefer bare 0 and 1. However, if the > driver writer wants to use TRUE/FALSE, I won't specifically reject it. > I really don't like the lower case true/false. > Me no understand. If you take the specific example of void ahd_set_syncrate(struct ahd_softc *ahd, struct ahd_devinfo *devinfo, u_int period, u_int offset, u_int ppr_options, u_int type, int paused) then if is crufty, inappropriate and wrong that `paused' is a scalar type. It's just not true or sensible that the code is written so that `paused' can take a value of seventy eight. It _is_ a boolean. It is a truth value. Declaring it as such in the source is all goodness. Passing the value `true' into calls to this function improve readability over passing "1". So I don't agree with (or understand) your objections. But I can certainly understand reluctance to merge a large-but-minor, do-nothing-much patch into a large and not-very-maintained driver. - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] drivers/scsi/aic7xxx_old: Convert to generic boolean-values
On Fri, 2007-02-16 at 10:34 -0800, Andrew Morton wrote: > On Fri, 16 Feb 2007 10:42:12 -0600 James Bottomley <[EMAIL PROTECTED]> wrote: > > > On Mon, 2007-02-12 at 12:27 -0800, Andrew Morton wrote: > > > Given that we now have a standard kernel-wide, c99-friendly way of > > > expressing true and false, I'd suggest that this decision can be > > > revisited. > > > > > > Because a "true" is significantly more meaningful (and hence readable) > > > thing than a bare "1". > > > > OK, I'm really not happy with doing this for three reasons: > > > > 1. It's inviting huge amounts of driver churn changing bitfields to > > booleans > > > > 2. I do find it to be a readability issue. Like most driver writers, > > I'm used to register layouts, and those are simple bitfields, so I don't > > tend to think true and false, I think 1 and 0. > > > > 3. Having a different, special, type for single bit bitfields (while > > still using u for multi bit bitfields) is asking for confusion, and > > hence trouble at the driver level. > > > > Confused. The patch changes TRUE to true and FALSE to false. The code > wasn't using bitfields before and isn't using them afterwards. I wouldn't > expect there to be any change in generated code. Sorry, I was addressing the general idea of using booleans in drivers. > All it's doing is replacing the driver's private TRUE/FALSE with the > kernel-wide ones. I already addressed that one ... I prefer bare 0 and 1. However, if the driver writer wants to use TRUE/FALSE, I won't specifically reject it. I really don't like the lower case true/false. James - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] drivers/scsi/aic7xxx_old: Convert to generic boolean-values
On Fri, 16 Feb 2007 10:42:12 -0600 James Bottomley <[EMAIL PROTECTED]> wrote: > On Mon, 2007-02-12 at 12:27 -0800, Andrew Morton wrote: > > Given that we now have a standard kernel-wide, c99-friendly way of > > expressing true and false, I'd suggest that this decision can be revisited. > > > > Because a "true" is significantly more meaningful (and hence readable) > > thing than a bare "1". > > OK, I'm really not happy with doing this for three reasons: > > 1. It's inviting huge amounts of driver churn changing bitfields to > booleans > > 2. I do find it to be a readability issue. Like most driver writers, > I'm used to register layouts, and those are simple bitfields, so I don't > tend to think true and false, I think 1 and 0. > > 3. Having a different, special, type for single bit bitfields (while > still using u for multi bit bitfields) is asking for confusion, and > hence trouble at the driver level. > Confused. The patch changes TRUE to true and FALSE to false. The code wasn't using bitfields before and isn't using them afterwards. I wouldn't expect there to be any change in generated code. All it's doing is replacing the driver's private TRUE/FALSE with the kernel-wide ones. - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] drivers/scsi/aic7xxx_old: Convert to generic boolean-values
On Fri, 2007-02-16 at 19:04 +0100, Richard Knutsson wrote: > James Bottomley wrote: > > On Mon, 2007-02-12 at 12:27 -0800, Andrew Morton wrote: > > > >> Given that we now have a standard kernel-wide, c99-friendly way of > >> expressing true and false, I'd suggest that this decision can be revisited. > >> > >> Because a "true" is significantly more meaningful (and hence readable) > >> thing than a bare "1". > >> > > > > OK, I'm really not happy with doing this for three reasons: > > > > 1. It's inviting huge amounts of driver churn changing bitfields to > > booleans > > > Have been some work done already. Has there been any problems? There's always an issue when two people work on the same driver ... it causes patch conflicts, which is why we try to avoid it where we can. > > 2. I do find it to be a readability issue. Like most driver writers, > > I'm used to register layouts, and those are simple bitfields, so I don't > > tend to think true and false, I think 1 and 0. > > > It is a fundamental difference between an integer and a boolean. Have > you seen anyone trying to do "bool var = true + true;"? ;) I don't quite see how this is relevant to the readability issue? > > 3. Having a different, special, type for single bit bitfields (while > > still using u for multi bit bitfields) is asking for confusion, and > > hence trouble at the driver level. > > > I don't think a boolean should be view as a single bit bitfield. Ex: > u8 a:1; > ... > int b = 4 + a; > is obviously not a boolean, while: > u8 a:1; > ... > if (a) > is, and a should be "bool a:1;" (imho) This again, doesn't really address the argument. I'm saying I'd rather not have confusion over what types to use in the driver. You're saying that if you only check the value for truth or falsehood it should be a boolean. That's actually worse than I was anticipating because you're now saying that single bit bitfields may or may not be booleans depending on use. This looks like worse potential for confusion than before. James - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] drivers/scsi/aic7xxx_old: Convert to generic boolean-values
James Bottomley wrote: On Mon, 2007-02-12 at 12:27 -0800, Andrew Morton wrote: Given that we now have a standard kernel-wide, c99-friendly way of expressing true and false, I'd suggest that this decision can be revisited. Because a "true" is significantly more meaningful (and hence readable) thing than a bare "1". OK, I'm really not happy with doing this for three reasons: 1. It's inviting huge amounts of driver churn changing bitfields to booleans Have been some work done already. Has there been any problems? 2. I do find it to be a readability issue. Like most driver writers, I'm used to register layouts, and those are simple bitfields, so I don't tend to think true and false, I think 1 and 0. It is a fundamental difference between an integer and a boolean. Have you seen anyone trying to do "bool var = true + true;"? ;) 3. Having a different, special, type for single bit bitfields (while still using u for multi bit bitfields) is asking for confusion, and hence trouble at the driver level. I don't think a boolean should be view as a single bit bitfield. Ex: u8 a:1; ... int b = 4 + a; is obviously not a boolean, while: u8 a:1; ... if (a) is, and a should be "bool a:1;" (imho) Richard Knutsson - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] drivers/scsi/aic7xxx_old: Convert to generic boolean-values
On Mon, 2007-02-12 at 12:27 -0800, Andrew Morton wrote: > Given that we now have a standard kernel-wide, c99-friendly way of > expressing true and false, I'd suggest that this decision can be revisited. > > Because a "true" is significantly more meaningful (and hence readable) > thing than a bare "1". OK, I'm really not happy with doing this for three reasons: 1. It's inviting huge amounts of driver churn changing bitfields to booleans 2. I do find it to be a readability issue. Like most driver writers, I'm used to register layouts, and those are simple bitfields, so I don't tend to think true and false, I think 1 and 0. 3. Having a different, special, type for single bit bitfields (while still using u for multi bit bitfields) is asking for confusion, and hence trouble at the driver level. James - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] drivers/scsi/aic7xxx_old: Convert to generic boolean-values
> On Sat, 10 Feb 2007 12:27:42 -0600 James Bottomley <[EMAIL PROTECTED]> wrote: > When discussion about TRUE and FALSE came up a long time a go in the > context of the mid layer we agreed to strip the defined constants out of > that code and just go with 1 and 0 inline ... because the code was > pretty much being rewritten. We also decided to encourage but not force > the driver writers simply to use 1 and 0 as well ... a lot of people are > deeply wedded to the TRUE and FALSE defines, it turned out. Given that we now have a standard kernel-wide, c99-friendly way of expressing true and false, I'd suggest that this decision can be revisited. Because a "true" is significantly more meaningful (and hence readable) thing than a bare "1". - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] drivers/scsi/aic7xxx_old: Convert to generic boolean-values
James Bottomley wrote: On Sat, 2007-02-10 at 18:46 +0100, Richard Knutsson wrote: Convert: FALSE -> false TRUE -> true Actually, downcasing true and false in this driver is pretty much a retrograde step. The reason for their being uppercased is that they represent constants (and uppercase is the traditional defined constant specifier). When discussion about TRUE and FALSE came up a long time a go in the context of the mid layer we agreed to strip the defined constants out of that code and just go with 1 and 0 inline ... because the code was pretty much being rewritten. We also decided to encourage but not force the driver writers simply to use 1 and 0 as well ... a lot of people are deeply wedded to the TRUE and FALSE defines, it turned out. Btw, is this just for aic7xxx_old and not aic7xxx? Is it going to be replaced? In that case, I will just leave it alone. Richard Knutsson - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] drivers/scsi/aic7xxx_old: Convert to generic boolean-values
James Bottomley wrote: On Sat, 2007-02-10 at 18:46 +0100, Richard Knutsson wrote: Convert: FALSE -> false TRUE -> true Actually, downcasing true and false in this driver is pretty much a retrograde step. The reason for their being uppercased is that they represent constants (and uppercase is the traditional defined constant specifier). I would argue that 'false' and 'true' are values and not constants, but further more C99 is defining them in lowercase (stdbool.h). When discussion about TRUE and FALSE came up a long time a go in the context of the mid layer we agreed to strip the defined constants out of that code and just go with 1 and 0 inline ... because the code was pretty much being rewritten. We also decided to encourage but not force the driver writers simply to use 1 and 0 as well ... a lot of people are deeply wedded to the TRUE and FALSE defines, it turned out. As I have expressed before, I don't understand why people seem to dislike 'false'/'true' but anyway, since you seem to approve booleans, would it be possible to convert the obvious variables/functions into boolean-type? Richard Knutsson - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] drivers/scsi/aic7xxx_old: Convert to generic boolean-values
On Sat, 2007-02-10 at 18:46 +0100, Richard Knutsson wrote: > Convert: > FALSE -> false > TRUE -> true Actually, downcasing true and false in this driver is pretty much a retrograde step. The reason for their being uppercased is that they represent constants (and uppercase is the traditional defined constant specifier). When discussion about TRUE and FALSE came up a long time a go in the context of the mid layer we agreed to strip the defined constants out of that code and just go with 1 and 0 inline ... because the code was pretty much being rewritten. We also decided to encourage but not force the driver writers simply to use 1 and 0 as well ... a lot of people are deeply wedded to the TRUE and FALSE defines, it turned out. James - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH] drivers/scsi/aic7xxx_old: Convert to generic boolean-values
Convert:
FALSE -> false
TRUE -> true
Signed-off-by: Richard Knutsson <[EMAIL PROTECTED]>
---
Compile-tested with "allyes", "allmod" & "allno" on i386
Whitespace cleaning on affected lines
drivers/scsi/aic7xxx_old.c | 242 +++-
drivers/scsi/aic7xxx_old/aic7xxx_proc.c |2
2 files changed, 119 insertions(+), 125 deletions(-)
diff --git a/drivers/scsi/aic7xxx_old.c b/drivers/scsi/aic7xxx_old.c
index 7d1fec6..92e32ee 100644
--- a/drivers/scsi/aic7xxx_old.c
+++ b/drivers/scsi/aic7xxx_old.c
@@ -256,12 +256,6 @@
#define ALL_LUNS -1
#define MAX_TARGETS 16
#define MAX_LUNS 8
-#ifndef TRUE
-# define TRUE 1
-#endif
-#ifndef FALSE
-# define FALSE 0
-#endif
#if defined(__powerpc__) || defined(__i386__) || defined(__x86_64__)
# define MMAPIO
@@ -1383,7 +1377,7 @@ aic7xxx_setup(char *s)
char *tok, *tok_end, *tok_end2;
char tok_list[] = { '.', ',', '{', '}', '\0' };
int i, instance = -1, device = -1;
-unsigned char done = FALSE;
+unsigned char done = false;
base = p;
tok = base + n + 1; /* Forward us just past the ':' */
@@ -1411,14 +1405,14 @@ aic7xxx_setup(char *s)
case ',':
case '.':
if (instance == -1)
-done = TRUE;
+done = true;
else if (device >= 0)
device++;
else if (instance >= 0)
instance++;
if ( (device >= MAX_TARGETS) ||
(instance >= ARRAY_SIZE(aic7xxx_tag_info)) )
-done = TRUE;
+done = true;
tok++;
if (!done)
{
@@ -1426,10 +1420,10 @@ aic7xxx_setup(char *s)
}
break;
case '\0':
- done = TRUE;
+ done = true;
break;
default:
- done = TRUE;
+ done = true;
tok_end = strchr(tok, '\0');
for(i=0; tok_list[i]; i++)
{
@@ -1437,7 +1431,7 @@ aic7xxx_setup(char *s)
if ( (tok_end2) && (tok_end2 < tok_end) )
{
tok_end = tok_end2;
- done = FALSE;
+ done = false;
}
}
if ( (instance >= 0) && (device >= 0) &&
@@ -1772,7 +1766,7 @@ aic7xxx_loadseq(struct aic7xxx_host *p)
aic_outb(p, 0, SEQADDR0);
aic_outb(p, 0, SEQADDR1);
aic_outb(p, FASTMODE | FAILDIS, SEQCTL);
- unpause_sequencer(p, TRUE);
+ unpause_sequencer(p, true);
mdelay(1);
pause_sequencer(p);
aic_outb(p, FASTMODE, SEQCTL);
@@ -1821,7 +1815,7 @@ aic7xxx_print_sequencer(struct aic7xxx_host *p, int
downloaded)
aic_outb(p, 0, SEQADDR0);
aic_outb(p, 0, SEQADDR1);
aic_outb(p, FASTMODE | FAILDIS, SEQCTL);
- unpause_sequencer(p, TRUE);
+ unpause_sequencer(p, true);
mdelay(1);
pause_sequencer(p);
aic_outb(p, FASTMODE, SEQCTL);
@@ -1869,7 +1863,7 @@ aic7xxx_find_syncrate(struct aic7xxx_host *p, unsigned
int *period,
unsigned int maxsync, unsigned char *options)
{
struct aic7xxx_syncrate *syncrate;
- int done = FALSE;
+ int done = false;
switch(*options)
{
@@ -1925,7 +1919,7 @@ aic7xxx_find_syncrate(struct aic7xxx_host *p, unsigned
int *period,
case MSG_EXT_PPR_OPTION_DT_UNITS:
if(!(syncrate->sxfr_ultra2 & AHC_SYNCRATE_CRC))
{
-done = TRUE;
+done = true;
/*
* oops, we went too low for the CRC/DualEdge signalling, so
* clear the options byte
@@ -1939,7 +1933,7 @@ aic7xxx_find_syncrate(struct aic7xxx_host *p, unsigned
int *period,
}
else
{
-done = TRUE;
+done = true;
if(syncrate == &aic7xxx_syncrates[maxsync])
{
*period = syncrate->period;
@@ -1949,7 +1943,7 @@ aic7xxx_find_syncrate(struct aic7xxx_host *p, unsigned
int *period,
default:
if(!(syncrate->sxfr_ultra2 & AHC_SYNCRATE_CRC))
{
-done = TRUE;
+done = true;
if(syncrate == &aic7xxx_syncrates[maxsync])
{
*period = syncrate->period;
@@ -2721,7 +2715,7 @@ aic7xxx_done(struct aic7xxx_host *p, struct aic7xxx_scb
*scb)
if ((scb->flags & SCB_MSGOUT_BITS) != 0)
{
unsigned short mask;
-int message_error = FALSE;
+int message_error = false;
mask = 0x01 << tindex;
@@ -2733,7 +2727,7 @@ aic7xxx_done(struct aic7xxx_host *p, struct aic7xxx_scb
*scb)
((scb->cmd->sense_buffer[12] == 0x43) || /* INVALID_MESSAGE */
(scb->cmd->sense_buffer[12] == 0x49))) /* MESSAGE_ERROR */
{
- message_error = TRUE;
