[KJ][Patch] fix coding style in fscpos.c

[KJ][Patch] fix coding style in fscpos.c

[Darren Jenkins\]

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

G'day list,

Here is a simple one I noticed.

The CodingStyle document says 
"Don't put multiple statements on a single line unless you have
something to hide:"

The patch below is a very simple change to convert to requisite coding
style.

Signed-off-by: Darren Jenkins <darrenrjenkins@gmail.com>
--- linux-2.6.16-rc3/drivers/hwmon/fscpos.c.orig	2006-02-22 23:14:39.000000000 +1100
+++ linux-2.6.16-rc3/drivers/hwmon/fscpos.c	2006-02-22 23:22:37.000000000 +1100
@@ -229,8 +229,10 @@ static ssize_t set_pwm(struct i2c_client
 	unsigned long v = simple_strtoul(buf, NULL, 10);
 
 	/* Range: 0..255 */
-	if (v < 0) v = 0;
-	if (v > 255) v = 255;
+	if (v < 0) 
+		v = 0;
+	if (v > 255) 
+		v = 255;
 
 	down(&data->update_lock);
 	data->pwm[nr - 1] = v;



[-- 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] fix coding style in fscpos.c

[Jean Delvare]

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

Hi Darren,

> Here is a simple one I noticed.
> 
> The CodingStyle document says 
> "Don't put multiple statements on a single line unless you have
> something to hide:"
> 
> The patch below is a very simple change to convert to requisite coding
> style.
> 
> Signed-off-by: Darren Jenkins <darrenrjenkins@gmail.com>
> --- linux-2.6.16-rc3/drivers/hwmon/fscpos.c.orig	2006-02-22 23:14:39.000000000 +1100
> +++ linux-2.6.16-rc3/drivers/hwmon/fscpos.c	2006-02-22 23:22:37.000000000 +1100
> @@ -229,8 +229,10 @@ static ssize_t set_pwm(struct i2c_client
>  	unsigned long v = simple_strtoul(buf, NULL, 10);
>  
>  	/* Range: 0..255 */
> -	if (v < 0) v = 0;
> -	if (v > 255) v = 255;
> +	if (v < 0) 
> +		v = 0;
> +	if (v > 255) 
> +		v = 255;
>  
>  	down(&data->update_lock);
>  	data->pwm[nr - 1] = v;
> 

Not worth the effort IMHO. The original code is pretty readable as it
is so I wouldn't change it.

-- 
Jean Delvare

[-- 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] fix coding style in fscpos.c

[Adrian Bunk]

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

On Wed, Feb 22, 2006 at 10:02:55PM +0100, Jean Delvare wrote:
> Hi Darren,
> 
> > Here is a simple one I noticed.
> > 
> > The CodingStyle document says 
> > "Don't put multiple statements on a single line unless you have
> > something to hide:"
> > 
> > The patch below is a very simple change to convert to requisite coding
> > style.
> > 
> > Signed-off-by: Darren Jenkins <darrenrjenkins@gmail.com>
> > --- linux-2.6.16-rc3/drivers/hwmon/fscpos.c.orig	2006-02-22 23:14:39.000000000 +1100
> > +++ linux-2.6.16-rc3/drivers/hwmon/fscpos.c	2006-02-22 23:22:37.000000000 +1100
> > @@ -229,8 +229,10 @@ static ssize_t set_pwm(struct i2c_client
> >  	unsigned long v = simple_strtoul(buf, NULL, 10);
> >  
> >  	/* Range: 0..255 */
> > -	if (v < 0) v = 0;
> > -	if (v > 255) v = 255;
> > +	if (v < 0) 
> > +		v = 0;
> > +	if (v > 255) 
> > +		v = 255;
> >  
> >  	down(&data->update_lock);
> >  	data->pwm[nr - 1] = v;
> > 
> 
> Not worth the effort IMHO. The original code is pretty readable as it
> is so I wouldn't change it.

It's against the kernel coding style.

There many several cases with several ways to express something pretty 
readable (opening braces being an religious example), but the goal is
to stick with one way to express something throughout the whole kernel.

> Jean Delvare

cu
Adrian

-- 

       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed


[-- 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] fix coding style in fscpos.c

[Jean Delvare]

Hi Adrian,

On 2006-02-23, Adrian Bunk wrote:
> > >  	/* Range: 0..255 */
> > > -	if (v < 0) v = 0;
> > > -	if (v > 255) v = 255;
> > > +	if (v < 0)
> > > +		v = 0;
> > > +	if (v > 255)
> > > +		v = 255;
> >
> > Not worth the effort IMHO. The original code is pretty readable as it
> > is so I wouldn't change it.
>
> It's against the kernel coding style.
>
> There many several cases with several ways to express something pretty
> readable (opening braces being an religious example), but the goal is
> to stick with one way to express something throughout the whole kernel.

I didn't mean to say otherwise. I request submitters of new drivers and
patches to comply with CodingStyle and even with the kernel guide to
space. I'm quite picky about it actually, just ask these submitters and
they'll tell you. I also wholeheartedly accept a cleanup patch,
including coding style, as the first patch of a patchset improving any
given driver, as it makes the next patches easier to work on and review.

However, I don't see much benefit in patches randomly fixing one coding
style mistake in a random driver, unless that mistake was making the
code especially unclear or error prone. The overhead to my workload and
in the driver history is simply not worth the gain. This was the meaning
of my reply.

--
Jean Delvare

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

Re: [KJ][Patch] fix coding style in fscpos.c

[Greg KH]

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

On Thu, Feb 23, 2006 at 09:57:36AM +0100, Jean Delvare wrote:
> 
> Hi Adrian,
> 
> On 2006-02-23, Adrian Bunk wrote:
> > > >  	/* Range: 0..255 */
> > > > -	if (v < 0) v = 0;
> > > > -	if (v > 255) v = 255;
> > > > +	if (v < 0)
> > > > +		v = 0;
> > > > +	if (v > 255)
> > > > +		v = 255;
> > >
> > > Not worth the effort IMHO. The original code is pretty readable as it
> > > is so I wouldn't change it.
> >
> > It's against the kernel coding style.
> >
> > There many several cases with several ways to express something pretty
> > readable (opening braces being an religious example), but the goal is
> > to stick with one way to express something throughout the whole kernel.
> 
> I didn't mean to say otherwise. I request submitters of new drivers and
> patches to comply with CodingStyle and even with the kernel guide to
> space. I'm quite picky about it actually, just ask these submitters and
> they'll tell you. I also wholeheartedly accept a cleanup patch,
> including coding style, as the first patch of a patchset improving any
> given driver, as it makes the next patches easier to work on and review.
> 
> However, I don't see much benefit in patches randomly fixing one coding
> style mistake in a random driver, unless that mistake was making the
> code especially unclear or error prone. The overhead to my workload and
> in the driver history is simply not worth the gain. This was the meaning
> of my reply.

That's exactly the point of the janitor project, to make changes like
this in places where needed.

Here, this is needed :)

thanks,

greg k-h

[-- 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] fix coding style in fscpos.c

[Randy.Dunlap]

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

On Thu, 23 Feb 2006, Greg KH wrote:

> On Thu, Feb 23, 2006 at 09:57:36AM +0100, Jean Delvare wrote:
> >
> > Hi Adrian,
> >
> > On 2006-02-23, Adrian Bunk wrote:
> > > > >  	/* Range: 0..255 */
> > > > > -	if (v < 0) v = 0;
> > > > > -	if (v > 255) v = 255;
> > > > > +	if (v < 0)
> > > > > +		v = 0;
> > > > > +	if (v > 255)
> > > > > +		v = 255;
> > > >
> > > > Not worth the effort IMHO. The original code is pretty readable as it
> > > > is so I wouldn't change it.
> > >
> > > It's against the kernel coding style.
> > >
> > > There many several cases with several ways to express something pretty
> > > readable (opening braces being an religious example), but the goal is
> > > to stick with one way to express something throughout the whole kernel.
> >
> > I didn't mean to say otherwise. I request submitters of new drivers and
> > patches to comply with CodingStyle and even with the kernel guide to
> > space. I'm quite picky about it actually, just ask these submitters and
> > they'll tell you. I also wholeheartedly accept a cleanup patch,
> > including coding style, as the first patch of a patchset improving any
> > given driver, as it makes the next patches easier to work on and review.
> >
> > However, I don't see much benefit in patches randomly fixing one coding
> > style mistake in a random driver, unless that mistake was making the
> > code especially unclear or error prone. The overhead to my workload and
> > in the driver history is simply not worth the gain. This was the meaning
> > of my reply.
>
> That's exactly the point of the janitor project, to make changes like
> this in places where needed.
>
> Here, this is needed :)

I think "needed" is too strong of a word there.
Like when my son says that he "needs" to go snowboarding
or to a movie.

It's OK to accept this patch, but not needed.
And CodingStyle is not a bible, it's more like a company's
policies and procedures manual, which (in good companies)
are just guidelines, not hard & fast rules.

and the KJ project is meant to train people to make useful
patches IMHO, not just to do busy work.

-- 
~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] fix coding style in fscpos.c

[Greg KH]

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

On Thu, Feb 23, 2006 at 09:58:04AM -0800, Randy.Dunlap wrote:
> On Thu, 23 Feb 2006, Greg KH wrote:
> 
> > On Thu, Feb 23, 2006 at 09:57:36AM +0100, Jean Delvare wrote:
> > >
> > > Hi Adrian,
> > >
> > > On 2006-02-23, Adrian Bunk wrote:
> > > > > >  	/* Range: 0..255 */
> > > > > > -	if (v < 0) v = 0;
> > > > > > -	if (v > 255) v = 255;
> > > > > > +	if (v < 0)
> > > > > > +		v = 0;
> > > > > > +	if (v > 255)
> > > > > > +		v = 255;
> > > > >
> > > > > Not worth the effort IMHO. The original code is pretty readable as it
> > > > > is so I wouldn't change it.
> > > >
> > > > It's against the kernel coding style.
> > > >
> > > > There many several cases with several ways to express something pretty
> > > > readable (opening braces being an religious example), but the goal is
> > > > to stick with one way to express something throughout the whole kernel.
> > >
> > > I didn't mean to say otherwise. I request submitters of new drivers and
> > > patches to comply with CodingStyle and even with the kernel guide to
> > > space. I'm quite picky about it actually, just ask these submitters and
> > > they'll tell you. I also wholeheartedly accept a cleanup patch,
> > > including coding style, as the first patch of a patchset improving any
> > > given driver, as it makes the next patches easier to work on and review.
> > >
> > > However, I don't see much benefit in patches randomly fixing one coding
> > > style mistake in a random driver, unless that mistake was making the
> > > code especially unclear or error prone. The overhead to my workload and
> > > in the driver history is simply not worth the gain. This was the meaning
> > > of my reply.
> >
> > That's exactly the point of the janitor project, to make changes like
> > this in places where needed.
> >
> > Here, this is needed :)
> 
> I think "needed" is too strong of a word there.
> Like when my son says that he "needs" to go snowboarding
> or to a movie.

Heh, good point :)

> It's OK to accept this patch, but not needed.
> And CodingStyle is not a bible, it's more like a company's
> policies and procedures manual, which (in good companies)
> are just guidelines, not hard & fast rules.

Agreed.

thanks,

greg k-h

[-- 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