[PATCH] staging: pi433: remove unnecessary parentheses

[PATCH] staging: pi433: remove unnecessary parentheses

[Valentin Vidic]

Fixes checkpatch warnings:

  CHECK: Unnecessary parentheses around 'mantisse != mantisse16'
  CHECK: Unnecessary parentheses around 'mantisse != mantisse20'
  CHECK: Unnecessary parentheses around 'mantisse != mantisse24'

Signed-off-by: Valentin Vidic <Valentin.Vidic@CARNet.hr>
---
 drivers/staging/pi433/rf69.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/staging/pi433/rf69.c b/drivers/staging/pi433/rf69.c
index bdd00f750765..a07fc6bc27f7 100644
--- a/drivers/staging/pi433/rf69.c
+++ b/drivers/staging/pi433/rf69.c
@@ -391,9 +391,9 @@ static int rf69_set_bandwidth_intern(struct spi_device *spi, u8 reg,
 		return -EINVAL;
 	}
 
-	if ((mantisse != mantisse16) &&
-	    (mantisse != mantisse20) &&
-	    (mantisse != mantisse24)) {
+	if (mantisse != mantisse16 &&
+	    mantisse != mantisse20 &&
+	    mantisse != mantisse24) {
 		dev_dbg(&spi->dev, "set: illegal input param");
 		return -EINVAL;
 	}
-- 
2.15.1

Re: [PATCH] staging: pi433: remove unnecessary parentheses

[Greg Kroah-Hartman]

On Mon, Jan 08, 2018 at 06:38:55PM +0100, Valentin Vidic wrote:
> Fixes checkpatch warnings:
> 
>   CHECK: Unnecessary parentheses around 'mantisse != mantisse16'
>   CHECK: Unnecessary parentheses around 'mantisse != mantisse20'
>   CHECK: Unnecessary parentheses around 'mantisse != mantisse24'
> 
> Signed-off-by: Valentin Vidic <Valentin.Vidic@CARNet.hr>
> ---
>  drivers/staging/pi433/rf69.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/staging/pi433/rf69.c b/drivers/staging/pi433/rf69.c
> index bdd00f750765..a07fc6bc27f7 100644
> --- a/drivers/staging/pi433/rf69.c
> +++ b/drivers/staging/pi433/rf69.c
> @@ -391,9 +391,9 @@ static int rf69_set_bandwidth_intern(struct spi_device *spi, u8 reg,
>  		return -EINVAL;
>  	}
>  
> -	if ((mantisse != mantisse16) &&
> -	    (mantisse != mantisse20) &&
> -	    (mantisse != mantisse24)) {
> +	if (mantisse != mantisse16 &&
> +	    mantisse != mantisse20 &&
> +	    mantisse != mantisse24) {

I'm getting really tired of seeing this checkpatch warning, when it's a
major pain.

Joe, can you please turn these off.  Patches like this will force people
to have to remember that != is higher precidence than &&.  The original
code here was just fine.

thanks,

greg k-h

>  		dev_dbg(&spi->dev, "set: illegal input param");
>  		return -EINVAL;
>  	}
> -- 
> 2.15.1
> 
> _______________________________________________
> devel mailing list
> devel@linuxdriverproject.org
> http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] staging: pi433: remove unnecessary parentheses

[Joe Perches]

On Tue, 2018-01-09 at 15:31 +0100, Greg Kroah-Hartman wrote:
> On Mon, Jan 08, 2018 at 06:38:55PM +0100, Valentin Vidic wrote:
> > Fixes checkpatch warnings:
> >   CHECK: Unnecessary parentheses around 'mantisse != mantisse16'
> >   CHECK: Unnecessary parentheses around 'mantisse != mantisse20'
> >   CHECK: Unnecessary parentheses around 'mantisse != mantisse24'
[]
> > diff --git a/drivers/staging/pi433/rf69.c b/drivers/staging/pi433/rf69.c
[]
> > @@ -391,9 +391,9 @@ static int rf69_set_bandwidth_intern(struct spi_device *spi, u8 reg,
> >  		return -EINVAL;
> >  	}
> >  
> > -	if ((mantisse != mantisse16) &&
> > -	    (mantisse != mantisse20) &&
> > -	    (mantisse != mantisse24)) {
> > +	if (mantisse != mantisse16 &&
> > +	    mantisse != mantisse20 &&
> > +	    mantisse != mantisse24) {
> 
> I'm getting really tired of seeing this checkpatch warning, when it's a
> major pain.

Your idea of major pain and mine differ a bit.

> Joe, can you please turn these off.  Patches like this will force people
> to have to remember that != is higher precidence than &&.

As it's not just 1 precedence level but 4 and 5, it
really shouldn't be that hard to remember.

> The original code here was just fine.

And I don't really disagree with you.

David Miller has a preference for the parenthesis free
style. I believe he mentioned it sometime in August 2017
but I can't find it right now.

Anyway, perhaps
---
 scripts/checkpatch.pl | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index d2464058ab5d..3a7499de2c2d 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -4526,7 +4526,9 @@ sub process {
 		}
 
 # check for unnecessary parentheses around comparisons in if uses
-		if ($^V && $^V ge 5.10.0 && defined($stat) &&
+# when !drivers/staging or the command-line uses --strict
+		if (($realfile !~ m@^(?:drivers/staging/)@ || $check_orig) &&
+		    $^V && $^V ge 5.10.0 && defined($stat) &&
 		    $stat =~ /(^.\s*if\s*($balanced_parens))/) {
 			my $if_stat = $1;
 			my $test = substr($2, 1, -1);

Re: [PATCH] staging: pi433: remove unnecessary parentheses

[Greg Kroah-Hartman]

On Tue, Jan 09, 2018 at 11:21:37AM -0800, Joe Perches wrote:
> On Tue, 2018-01-09 at 15:31 +0100, Greg Kroah-Hartman wrote:
> > On Mon, Jan 08, 2018 at 06:38:55PM +0100, Valentin Vidic wrote:
> > > Fixes checkpatch warnings:
> > >   CHECK: Unnecessary parentheses around 'mantisse != mantisse16'
> > >   CHECK: Unnecessary parentheses around 'mantisse != mantisse20'
> > >   CHECK: Unnecessary parentheses around 'mantisse != mantisse24'
> []
> > > diff --git a/drivers/staging/pi433/rf69.c b/drivers/staging/pi433/rf69.c
> []
> > > @@ -391,9 +391,9 @@ static int rf69_set_bandwidth_intern(struct spi_device *spi, u8 reg,
> > >  		return -EINVAL;
> > >  	}
> > >  
> > > -	if ((mantisse != mantisse16) &&
> > > -	    (mantisse != mantisse20) &&
> > > -	    (mantisse != mantisse24)) {
> > > +	if (mantisse != mantisse16 &&
> > > +	    mantisse != mantisse20 &&
> > > +	    mantisse != mantisse24) {
> > 
> > I'm getting really tired of seeing this checkpatch warning, when it's a
> > major pain.
> 
> Your idea of major pain and mine differ a bit.

I don't like taking patches that cause future problems.

> > Joe, can you please turn these off.  Patches like this will force people
> > to have to remember that != is higher precidence than &&.
> 
> As it's not just 1 precedence level but 4 and 5, it
> really shouldn't be that hard to remember.

I can't remember any of them, and I should not have to.  That's the
point, you should not assume anyone knows the levels, code is written
for developers to understand first, and the compiler second.  By
removing these, it doesn't do anything for the compiler, but makes the
developer think longer about them.

> > The original code here was just fine.
> 
> And I don't really disagree with you.
> 
> David Miller has a preference for the parenthesis free
> style. I believe he mentioned it sometime in August 2017
> but I can't find it right now.
> 
> Anyway, perhaps
> ---
>  scripts/checkpatch.pl | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index d2464058ab5d..3a7499de2c2d 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -4526,7 +4526,9 @@ sub process {
>  		}
>  
>  # check for unnecessary parentheses around comparisons in if uses
> -		if ($^V && $^V ge 5.10.0 && defined($stat) &&
> +# when !drivers/staging or the command-line uses --strict
> +		if (($realfile !~ m@^(?:drivers/staging/)@ || $check_orig) &&
> +		    $^V && $^V ge 5.10.0 && defined($stat) &&

How about only if in the networking area?  I don't want to take patches
for this for other parts of the kernel either, it's really useless.

thanks,

greg k-h

Re: [PATCH] staging: pi433: remove unnecessary parentheses

[Joe Perches]

On Tue, 2018-01-09 at 20:28 +0100, Greg Kroah-Hartman wrote:
> On Tue, Jan 09, 2018 at 11:21:37AM -0800, Joe Perches wrote:
> > On Tue, 2018-01-09 at 15:31 +0100, Greg Kroah-Hartman wrote:
> > > On Mon, Jan 08, 2018 at 06:38:55PM +0100, Valentin Vidic wrote:
> > > > Fixes checkpatch warnings:
> > > >   CHECK: Unnecessary parentheses around 'mantisse != mantisse16'
> > > >   CHECK: Unnecessary parentheses around 'mantisse != mantisse20'
> > > >   CHECK: Unnecessary parentheses around 'mantisse != mantisse24'
> > 
> > []
> > > > diff --git a/drivers/staging/pi433/rf69.c b/drivers/staging/pi433/rf69.c
> > 
> > []
> > > > @@ -391,9 +391,9 @@ static int rf69_set_bandwidth_intern(struct spi_device *spi, u8 reg,
> > > >  		return -EINVAL;
> > > >  	}
> > > >  
> > > > -	if ((mantisse != mantisse16) &&
> > > > -	    (mantisse != mantisse20) &&
> > > > -	    (mantisse != mantisse24)) {
> > > > +	if (mantisse != mantisse16 &&
> > > > +	    mantisse != mantisse20 &&
> > > > +	    mantisse != mantisse24) {
> > > 
> > > I'm getting really tired of seeing this checkpatch warning, when it's a
> > > major pain.
> > 
> > Your idea of major pain and mine differ a bit.
> 
> I don't like taking patches that cause future problems.

What future problems might this particular case present
that isn't generic in all patches.

> > > Joe, can you please turn these off.  Patches like this will force people
> > > to have to remember that != is higher precidence than &&.
> > 
> > As it's not just 1 precedence level but 4 and 5, it
> > really shouldn't be that hard to remember.
> 
> I can't remember any of them, and I should not have to.

That depends on how well you know your C.

>   That's the
> point, you should not assume anyone knows the levels, code is written
> for developers to understand first, and the compiler second.

And someone that knows C knows those levels and the parentheses
can just be visual noise requiring extra thought.

Sometimes it's useful, sometimes it's not.

	if (a == b && c == d)

is pretty trivial.

and I believe

	if ((a == b))

emits clang warnings

>   By
> removing these, it doesn't do anything for the compiler, but makes the
> developer think longer about them.

Generally I have to think more with more parentheses.

> > > The original code here was just fine.
> > 
> > And I don't really disagree with you.
> > 
> > David Miller has a preference for the parenthesis free
> > style. I believe he mentioned it sometime in August 2017
> > but I can't find it right now.
> > 
> > Anyway, perhaps
> > ---
> >  scripts/checkpatch.pl | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> > 
> > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> > index d2464058ab5d..3a7499de2c2d 100755
> > --- a/scripts/checkpatch.pl
> > +++ b/scripts/checkpatch.pl
> > @@ -4526,7 +4526,9 @@ sub process {
> >  		}
> >  
> >  # check for unnecessary parentheses around comparisons in if uses
> > -		if ($^V && $^V ge 5.10.0 && defined($stat) &&
> > +# when !drivers/staging or the command-line uses --strict
> > +		if (($realfile !~ m@^(?:drivers/staging/)@ || $check_orig) &&
> > +		    $^V && $^V ge 5.10.0 && defined($stat) &&
> 
> How about only if in the networking area?  I don't want to take patches
> for this for other parts of the kernel either, it's really useless.

AFAIK: Almost no one thinks when sending staging patches.

For other parts of the tree, it requires --strict on the
command line.  AFAIK: almost no one uses that and if they
do, then they can determine for themselves if they want
to remove excessive parentheses.

Re: [PATCH] staging: pi433: remove unnecessary parentheses

[Greg Kroah-Hartman]

On Tue, Jan 09, 2018 at 11:42:16AM -0800, Joe Perches wrote:
> On Tue, 2018-01-09 at 20:28 +0100, Greg Kroah-Hartman wrote:
> > On Tue, Jan 09, 2018 at 11:21:37AM -0800, Joe Perches wrote:
> > > On Tue, 2018-01-09 at 15:31 +0100, Greg Kroah-Hartman wrote:
> > > > On Mon, Jan 08, 2018 at 06:38:55PM +0100, Valentin Vidic wrote:
> > > > > Fixes checkpatch warnings:
> > > > >   CHECK: Unnecessary parentheses around 'mantisse != mantisse16'
> > > > >   CHECK: Unnecessary parentheses around 'mantisse != mantisse20'
> > > > >   CHECK: Unnecessary parentheses around 'mantisse != mantisse24'
> > > 
> > > []
> > > > > diff --git a/drivers/staging/pi433/rf69.c b/drivers/staging/pi433/rf69.c
> > > 
> > > []
> > > > > @@ -391,9 +391,9 @@ static int rf69_set_bandwidth_intern(struct spi_device *spi, u8 reg,
> > > > >  		return -EINVAL;
> > > > >  	}
> > > > >  
> > > > > -	if ((mantisse != mantisse16) &&
> > > > > -	    (mantisse != mantisse20) &&
> > > > > -	    (mantisse != mantisse24)) {
> > > > > +	if (mantisse != mantisse16 &&
> > > > > +	    mantisse != mantisse20 &&
> > > > > +	    mantisse != mantisse24) {
> > > > 
> > > > I'm getting really tired of seeing this checkpatch warning, when it's a
> > > > major pain.
> > > 
> > > Your idea of major pain and mine differ a bit.
> > 
> > I don't like taking patches that cause future problems.
> 
> What future problems might this particular case present
> that isn't generic in all patches.
> 
> > > > Joe, can you please turn these off.  Patches like this will force people
> > > > to have to remember that != is higher precidence than &&.
> > > 
> > > As it's not just 1 precedence level but 4 and 5, it
> > > really shouldn't be that hard to remember.
> > 
> > I can't remember any of them, and I should not have to.
> 
> That depends on how well you know your C.

I have used C for almost ever single day for the past 20+ years, and I
sure don't remember the order of these things.  But maybe I really don't
know my C :)

> >   That's the
> > point, you should not assume anyone knows the levels, code is written
> > for developers to understand first, and the compiler second.
> 
> And someone that knows C knows those levels and the parentheses
> can just be visual noise requiring extra thought.
> 
> Sometimes it's useful, sometimes it's not.
> 
> 	if (a == b && c == d)
> 
> is pretty trivial.

But again, don't do that.

> and I believe
> 
> 	if ((a == b))
> 
> emits clang warnings

Then remove the extra () there.

thanks,

greg k-h

Re: [PATCH] staging: pi433: remove unnecessary parentheses

[Joe Perches]

On Wed, 2018-01-10 at 09:44 +0100, Greg Kroah-Hartman wrote:
> On Tue, Jan 09, 2018 at 11:42:16AM -0800, Joe Perches wrote:
> > 	if (a == b && c == d)
> > is pretty trivial.
> 
> But again, don't do that.

<shrug>  We disagree.  Life goes on.

cheers, Joe

Re: [PATCH] staging: pi433: remove unnecessary parentheses

[marcus.wolf]

Joe Perches schrieb am 10.01.2018 10:05:
> On Wed, 2018-01-10 at 09:44 +0100, Greg Kroah-Hartman wrote:
> > On Tue, Jan 09, 2018 at 11:42:16AM -0800, Joe Perches wrote:
> > > if (a == b && c == d)
> > > is pretty trivial.
> >
> > But again, don't do that.
> 
> <shrug> We disagree. Life goes on.
> 
> cheers, Joe
>
>

For me the line above isn't obvious and easy to read. If I would be in doubt, whether it really performs correctly, I would have to ask the c-guide, to be absolutely shure.
But to be honest: If I need to find a bug arround taht lines, I wouldn't ask the c-guide, but simply add some (). Then it would be 100% clear and no one would be in doubt any more.

What's the disadvantage of () to emphasise waht is going on. An other Option for me would be, to spend a command line and write that info in form of a comment.


Just my opinion and the way, I would go on if I am in doubt and need to find a bug.

Cheers,

Marcus