[KJ][Patch] remove implicit sign bit in msp3400.h

[KJ][Patch] remove implicit sign bit in msp3400.h

[Darren Jenkins\]

[-- Attachment #1: Type: text/plain, Size: 1003 bytes --]

G'day

the two fields here

	int restart:1;
	int watch_stereo:1;

they both seem to be used as single bit flags. So this patch just makes
them unsigned.

Note : Not sure if something should be done in msp3400-driver.c as 
"int msp_sleep(struct msp_state *state, int timeout)"
is declared to return an int then returns
"return state->restart;" which is now "unsigned:1;"
This seems ok but I don't know if gcc will issue a warning without a
cast.
Note: if anyone can tell me if it's possible/how to compile
msp3400-driver.c by its self that would be handy.



--- linux-2.6.16-rc2/drivers/media/video/msp3400.h.orig	2006-02-08 15:57:02.000000000 +1100
+++ linux-2.6.16-rc2/drivers/media/video/msp3400.h	2006-02-08 17:09:45.000000000 +1100
@@ -86,8 +86,8 @@ struct msp_state {
 	/* thread */
 	struct task_struct   *kthread;
 	wait_queue_head_t    wq;
-	int                  restart:1;
-	int                  watch_stereo:1;
+	unsigned restart:1;
+	unsigned watch_stereo:1;
 };
 
 /* msp3400-driver.c */



[-- Attachment #2: Type: text/plain, Size: 168 bytes --]

_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors

[KJ][Patch] remove implicit sign bit in l64781.c

[Darren Jenkins\]

[-- Attachment #1: Type: text/plain, Size: 631 bytes --]

G'day 

Below is a patch which removes the implicit sign bit on a single bit
field. The field 'first' is used as a flag to indicate a first attempt
at init. So the patch just converts it to unsigned.


Signed-off-by: Darren Jenkins <darrenrjenkins@gmail.com>

--- linux-2.6.16-rc2/drivers/media/dvb/frontends/l64781.c.orig	2006-02-08 17:51:35.000000000 +1100
+++ linux-2.6.16-rc2/drivers/media/dvb/frontends/l64781.c	2006-02-08 17:53:34.000000000 +1100
@@ -37,7 +37,7 @@ struct l64781_state {
 	struct dvb_frontend frontend;
 
 	/* private demodulator data */
-	int first:1;
+	unsigned first:1;
 };
 
 #define dprintk(args...) \



[-- Attachment #2: Type: text/plain, Size: 168 bytes --]

_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors

[KJ][Patch] remove implicit sign bit in isdnhdlc.h

[Darren Jenkins\]

[-- Attachment #1: Type: text/plain, Size: 1086 bytes --]

G'day list.

This patch removes the implicit sign bit in a couple of one bit struct
fields.
The fields here are used as one bit flags. So the patch just makes them
unsigned. As a bonus it converts spaces to a tab before the last field.

Signed-off-by: Darren Jenkins <darrenrjenkins@gmail.com>

--- linux-2.6.16-rc2/drivers/isdn/hisax/isdnhdlc.h.orig	2006-02-08 18:05:43.000000000 +1100
+++ linux-2.6.16-rc2/drivers/isdn/hisax/isdnhdlc.h	2006-02-08 18:09:01.000000000 +1100
@@ -41,10 +41,10 @@ struct isdnhdlc_vars {
 	unsigned char shift_reg;
 	unsigned char ffvalue;
 
-	int data_received:1; 	// set if transferring data
-	int dchannel:1; 	// set if D channel (send idle instead of flags)
-	int do_adapt56:1; 	// set if 56K adaptation
-        int do_closing:1; 	// set if in closing phase (need to send CRC + flag
+	unsigned data_received:1; 	// set if transferring data
+	unsigned dchannel:1; 	// set if D channel (send idle instead of flags)
+	unsigned do_adapt56:1; 	// set if 56K adaptation
+	unsigned do_closing:1; 	// set if in closing phase (need to send CRC + flag
 };
 
 



[-- Attachment #2: Type: text/plain, Size: 168 bytes --]

_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors

Re: [KJ][Patch] remove implicit sign bit in msp3400.h

[Randy.Dunlap]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1195 bytes --]

On Wed, 8 Feb 2006, Darren Jenkins\ wrote:

> G'day
>
> the two fields here
>
> 	int restart:1;
> 	int watch_stereo:1;
>
> they both seem to be used as single bit flags. So this patch just makes
> them unsigned.
>
> Note : Not sure if something should be done in msp3400-driver.c as
> "int msp_sleep(struct msp_state *state, int timeout)"
> is declared to return an int then returns
> "return state->restart;" which is now "unsigned:1;"
> This seems ok but I don't know if gcc will issue a warning without a
> cast.
> Note: if anyone can tell me if it's possible/how to compile
> msp3400-driver.c by its self that would be handy.
>
>
>
> --- linux-2.6.16-rc2/drivers/media/video/msp3400.h.orig	2006-02-08 15:57:02.000000000 +1100
> +++ linux-2.6.16-rc2/drivers/media/video/msp3400.h	2006-02-08 17:09:45.000000000 +1100
> @@ -86,8 +86,8 @@ struct msp_state {
>  	/* thread */
>  	struct task_struct   *kthread;
>  	wait_queue_head_t    wq;
> -	int                  restart:1;
> -	int                  watch_stereo:1;
> +	unsigned restart:1;
> +	unsigned watch_stereo:1;
>  };

Please keep the same spacing/tab/alignment.
And usually "unsigned int" is preferred in kernel source code.

-- 
~Randy

[-- Attachment #2: Type: text/plain, Size: 168 bytes --]

_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors

Re: [KJ][Patch] remove implicit sign bit in msp3400.h

[Darren Jenkins\]

[-- Attachment #1: Type: text/plain, Size: 1020 bytes --]

On Wed, 2006-02-08 at 07:32 -0800, Randy.Dunlap wrote:
> Please keep the same spacing/tab/alignment.

Fair enough

> And usually "unsigned int" is preferred in kernel source code.

After doing a grep on the kernel I have to agree, most of them are
"unsigned int foo :1;" 
Weird if you ask me. I'm pretty sure the C standard says they are
promoted to an int for calculations. But I suppose as it makes no
difference it is better to stick with convention.

So here is a re-send of the patch.


Signed-off-by: Darren Jenkins <darrenrjenkins@gmail.com>


--- linux-2.6.16-rc2/drivers/media/video/msp3400.h.orig	2006-02-08 15:57:02.000000000 +1100
+++ linux-2.6.16-rc2/drivers/media/video/msp3400.h	2006-02-09 22:55:41.000000000 +1100
@@ -86,8 +86,8 @@ struct msp_state {
 	/* thread */
 	struct task_struct   *kthread;
 	wait_queue_head_t    wq;
-	int                  restart:1;
-	int                  watch_stereo:1;
+	unsigned int         restart:1;
+	unsigned int         watch_stereo:1;
 };
 
 /* msp3400-driver.c */



[-- Attachment #2: Type: text/plain, Size: 168 bytes --]

_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors

Re: [KJ][Patch] remove implicit sign bit in msp3400.h

[Alexey Dobriyan]

[-- Attachment #1: Type: text/plain, Size: 1178 bytes --]

On Thu, Feb 09, 2006 at 11:15:31PM +1100, Darren Jenkins" wrote:
> Weird if you ask me. I'm pretty sure the C standard says they are
> promoted to an int for calculations.

It's very logical.

	int a:1;

means 1 bit for sign (it's int) + >=1 bit for absolute value (it's int).
So, >=2 bits you should somehow fit into 1 bit.

	int	   |	unsigned int   |
-------------------+-------------------+---
doesn't make sense |	     OK	       |:1
	OK	   |	     OK	       |:2
	OK	   |	     OK	       |:3
			...

I guess C standard says "undefined behaviour" or "implementation-defined".

> But I suppose as it makes no
> difference it is better to stick with convention.

> --- linux-2.6.16-rc2/drivers/media/video/msp3400.h.orig
> +++ linux-2.6.16-rc2/drivers/media/video/msp3400.h
> @@ -86,8 +86,8 @@ struct msp_state {
>  	/* thread */
>  	struct task_struct   *kthread;
>  	wait_queue_head_t    wq;
> -	int                  restart:1;
> -	int                  watch_stereo:1;
> +	unsigned int         restart:1;
> +	unsigned int         watch_stereo:1;

Applied.

BTW, someone could do

	int a:1;
	if (a < 0)

so every time type is flipped, submitter should check every usage of "a".


[-- Attachment #2: Type: text/plain, Size: 168 bytes --]

_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors

Re: [KJ][Patch] remove implicit sign bit in msp3400.h

[Darren Jenkins\]

[-- Attachment #1: Type: text/plain, Size: 1149 bytes --]

On Sat, 2006-02-18 at 15:33 +0300, Alexey Dobriyan wrote:
> On Thu, Feb 09, 2006 at 11:15:31PM +1100, Darren Jenkins" wrote:
> > Weird if you ask me. I'm pretty sure the C standard says they are
> > promoted to an int for calculations.
> 
> It's very logical.
> 
> 	int a:1;
> 
> means 1 bit for sign (it's int) + >=1 bit for absolute value (it's int).
> So, >=2 bits you should somehow fit into 1 bit.
> 
> 	int	   |	unsigned int   |
> -------------------+-------------------+---
> doesn't make sense |	     OK	       |:1
> 	OK	   |	     OK	       |:2
> 	OK	   |	     OK	       |:3
> 			...
> 
> I guess C standard says "undefined behaviour" or "implementation-defined".
Yep, that's how I understand it.

What I thought was weird was that the common coding style in the kernel
was to include the int, when it is implicit. Where the rest of the style
in the kernel seems to be to remove anything that is implicit. 
Just an observation.

> BTW, someone could do
> 
> 	int a:1;
> 	if (a < 0)
> 
> so every time type is flipped, submitter should check every usage of "a".
> 

I did not think of that kind of usage. Will check it in future.

Darren J.


[-- Attachment #2: Type: text/plain, Size: 168 bytes --]

_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors

Re: [KJ][Patch] remove implicit sign bit in msp3400.h

[Matthew Wilcox]

[-- Attachment #1: Type: text/plain, Size: 1077 bytes --]

On Sat, Feb 18, 2006 at 03:33:12PM +0300, Alexey Dobriyan wrote:
> It's very logical.
> 
> 	int a:1;
> 
> means 1 bit for sign (it's int) + >=1 bit for absolute value (it's int).
> So, >=2 bits you should somehow fit into 1 bit.

Standard 2's-complement representation.  The MSB is negative.  So
instead of the bits meaning 1, 2, 4, 8, 16, 32, 64, 128, the top bit
represents -128.  So the one bit case represents -1.

ie the two values it can have are 0 and -1.

> I guess C standard says "undefined behaviour" or "implementation-defined".

It does ... sort of:

6.7.2:
for bit-fields, it is implementation-defined whether the specifier int
designates the same type as signed int or the same type as unsigned int.

so relying on int to be signed was already broken.

> BTW, someone could do
> 
> 	int a:1;
> 	if (a < 0)
> 
> so every time type is flipped, submitter should check every usage of "a".
> 

> _______________________________________________
> Kernel-janitors mailing list
> Kernel-janitors@lists.osdl.org
> https://lists.osdl.org/mailman/listinfo/kernel-janitors


[-- Attachment #2: Type: text/plain, Size: 168 bytes --]

_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors