[PATCH] scripts/checkpatch.pl: Add warning about leading contination tests

[PATCH] scripts/checkpatch.pl: Add warning about leading contination tests

[Joe Perches]

Signed-off-by: Joe Perches <joe@perches.com>

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index bc4114f..c35933a 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -2064,6 +2064,11 @@ sub process {
 			CHK("multiple assignments should be avoided\n" . $herecurr);
 		}
 
+# Check use of leading logical continuation tests
+		if ($line =~ /^.\s*(\|\||&&)/) {
+			WARN("Continuation logic should be at end of previous line\n" . $herecurr);
+		}
+
 ## # check for multiple declarations, allowing for a function declaration
 ## # continuation.
 ## 		if ($line =~ /^.\s*$Type\s+$Ident(?:\s*=[^,{]*)?\s*,\s*$Ident.*/ &&

Re: [PATCH] scripts/checkpatch.pl: Add warning about leading contination tests

[Eric Dumazet]

Joe Perches a écrit :
> Signed-off-by: Joe Perches <joe@perches.com>
> 
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index bc4114f..c35933a 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -2064,6 +2064,11 @@ sub process {
>  			CHK("multiple assignments should be avoided\n" . $herecurr);
>  		}
>  
> +# Check use of leading logical continuation tests
> +		if ($line =~ /^.\s*(\|\||&&)/) {
> +			WARN("Continuation logic should be at end of previous line\n" . $herecurr);
> +		}
> +
>  ## # check for multiple declarations, allowing for a function declaration
>  ## # continuation.
>  ## 		if ($line =~ /^.\s*$Type\s+$Ident(?:\s*=[^,{]*)?\s*,\s*$Ident.*/ &&
> 
> 

Fine with me, but please add relevant info in Documentation/CodingStyle ?

Re: [PATCH] scripts/checkpatch.pl: Add warning about leading contination tests

[Jean Delvare]

On Sun, 06 Dec 2009 09:35:04 +0100, Eric Dumazet wrote:
> Joe Perches a écrit :
> > Signed-off-by: Joe Perches <joe@perches.com>
> > 
> > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> > index bc4114f..c35933a 100755
> > --- a/scripts/checkpatch.pl
> > +++ b/scripts/checkpatch.pl
> > @@ -2064,6 +2064,11 @@ sub process {
> >  			CHK("multiple assignments should be avoided\n" . $herecurr);
> >  		}
> >  
> > +# Check use of leading logical continuation tests
> > +		if ($line =~ /^.\s*(\|\||&&)/) {
> > +			WARN("Continuation logic should be at end of previous line\n" . $herecurr);
> > +		}
> > +
> >  ## # check for multiple declarations, allowing for a function declaration
> >  ## # continuation.
> >  ## 		if ($line =~ /^.\s*$Type\s+$Ident(?:\s*=[^,{]*)?\s*,\s*$Ident.*/ &&
> > 
> > 
> 
> Fine with me, but please add relevant info in Documentation/CodingStyle ?

Not fine with me. Placing the binary logic operator at the beginning
of a line can be a deliberate choice, either to make complex binary
expressions more readable, or to avoid long lines. I don't see much
point in banning this style, which BTW is used over 8000 times in the
current kernel tree.

-- 
Jean Delvare

Re: [PATCH] scripts/checkpatch.pl: Add warning about leading contination tests

[Joe Perches]

On Sun, 2009-12-06 at 13:13 +0100, Jean Delvare wrote:
> Not fine with me. Placing the binary logic operator at the beginning
> of a line can be a deliberate choice, either to make complex binary
> expressions more readable, or to avoid long lines. I don't see much
> point in banning this style, which BTW is used over 8000 times in the
> current kernel tree.

Anyone that thinks that checkpatch is the
last word on linux coding style and all of
its pronouncements must be followed all the
time is simply wrong.

It's not a ban.  It's neither a command nor
an edict.  It's a warning.  It's a notice
that leading logical continuations are not
the preferred style and it can be ignored
at will.

I think it's rather like the long line, >80
column warning.  There are a whole lot more
than 8k long lines in kernel source and no
one is suggesting reformatting all of them
out of existence.

cheers, Joe

Re: [PATCH] scripts/checkpatch.pl: Add warning about leading contination tests

[Jean Delvare]

On Sun, 06 Dec 2009 09:46:16 -0800, Joe Perches wrote:
> On Sun, 2009-12-06 at 13:13 +0100, Jean Delvare wrote:
> > Not fine with me. Placing the binary logic operator at the beginning
> > of a line can be a deliberate choice, either to make complex binary
> > expressions more readable, or to avoid long lines. I don't see much
> > point in banning this style, which BTW is used over 8000 times in the
> > current kernel tree.
> 
> Anyone that thinks that checkpatch is the
> last word on linux coding style and all of
> its pronouncements must be followed all the
> time is simply wrong.
> 
> It's not a ban.  It's neither a command nor
> an edict.  It's a warning.  It's a notice
> that leading logical continuations are not
> the preferred style and it can be ignored
> at will.

I don't disagree with that. However, adding more and more checks in
checkpatch.pl has its downsides. For example, I want to be able to tell
people who submit patches to me: "run checkpatch.pl on your patch and
solve every problem it reports before sending it to me again". If I
must instead tell them: "run checkpatch.pl on your patch and fix what
you want" then they might as well not fix anything, because they will
not know which warnings _I_ find relevant and which I don't. Then the
checkpatch.pl script becomes useless for that use case.

More generally, if checkpatch.pl starts reporting too many warnings
which people consider false positives, then developers may simply stop
using it. This especially holds for newcomers. If they check their
patch and they get 10 errors or warnings, they'll fix them. If they get
100 they may just give up. checkpatch.pl is a wonderful tool and I
don't want to lose that.

So if you are going to add checks which are icing on the cake, please
disable them by default and only show them if the user explicitly asks
for it by passing --strict or some such. As a matter of fact, the rule
you are trying to add is not in Documentation/CodingStyle, as Eric
noticed already, so it can't be that important, can it?

> I think it's rather like the long line, >80
> column warning.  There are a whole lot more
> than 8k long lines in kernel source and no
> one is suggesting reformatting all of them
> out of existence.

Lines longer than 8_k_? I hope not ;)

-- 
Jean Delvare

Re: [PATCH] scripts/checkpatch.pl: Add warning about leading contination tests

[Joe Perches]

On Sun, 2009-12-06 at 19:53 +0100, Jean Delvare wrote:
> I want to be able to tell
> people who submit patches to me: "run checkpatch.pl on your patch and
> solve every problem it reports before sending it to me again". If I
> must instead tell them: "run checkpatch.pl on your patch and fix what
> you want" then they might as well not fix anything, because they will
> not know which warnings _I_ find relevant and which I don't. Then the
> checkpatch.pl script becomes useless for that use case.

If you actually do that, you probably want them
to fix _every last problem_ because the patch is
either trivial or has so many broken elements
that asking that contributor to fix them all is
a learning experience for them.

> So if you are going to add checks which are icing on the cake, please
> disable them by default and only show them if the user explicitly asks

Making the test use the CHK function rather
than WARNING one seems sensible.

> > I think it's rather like the long line, >80
> > column warning.  There are a whole lot more
> > than 8k long lines in kernel source and no
> > one is suggesting reformatting all of them
> > out of existence.
> 
> Lines longer than 8_k_? I hope not ;)

Interpretive reading is like interpretive dance.
I've used compilers like that...

cheers, Joe

Re: [PATCH] scripts/checkpatch.pl: Add warning about leading contination tests

[J. Bruce Fields]

On Sat, Dec 05, 2009 at 09:58:04AM -0800, Joe Perches wrote:
> Signed-off-by: Joe Perches <joe@perches.com>
> 
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index bc4114f..c35933a 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -2064,6 +2064,11 @@ sub process {
>  			CHK("multiple assignments should be avoided\n" . $herecurr);
>  		}
>  
> +# Check use of leading logical continuation tests
> +		if ($line =~ /^.\s*(\|\||&&)/) {
> +			WARN("Continuation logic should be at end of previous line\n" . $herecurr);
> +		}
> +
>  ## # check for multiple declarations, allowing for a function declaration
>  ## # continuation.
>  ## 		if ($line =~ /^.\s*$Type\s+$Ident(?:\s*=[^,{]*)?\s*,\s*$Ident.*/ &&

Where does this preference come from?

In

	excessivelylongcondition
	&& anotherreallylongcondition
	&& yetanotherunbelievablylongcondition
	&& yetanotherwellyougettheidea

I want to be able to keep the &&'s all justified.

Or look for well-typeset math or CS texts and try to find any that leave
operators dangling on the right.

I don't really care much about this particular point, but: the
checkpatch output is already getting too verbose to be useful, without
adding advice that's actually the opposite of what I'd normally want to
do....

--b.

Re: [PATCH] scripts/checkpatch.pl: Add warning about leading contination tests

[William Allen Simpson]

J. Bruce Fields wrote:
> Where does this preference come from?
> 
David Miller -- in response to a patch of mine that used:
  - trailing && on existing lines that already had trailing &&, and
  - leading && on existing lines that already had leading &&, and
  - leading && on new code.

He decided he wants "consistency", existing code be damned.


> In
> 
> 	excessivelylongcondition
> 	&& anotherreallylongcondition
> 	&& yetanotherunbelievablylongcondition
> 	&& yetanotherwellyougettheidea
> 
> I want to be able to keep the &&'s all justified.
> 
Agree with you and Jean Delvare and thousands of other developers.


> Or look for well-typeset math or CS texts and try to find any that leave
> operators dangling on the right.
> 
Agreed.


> I don't really care much about this particular point, but: the
> checkpatch output is already getting too verbose to be useful, without
> adding advice that's actually the opposite of what I'd normally want to
> do....
> 
Yes, you are agreeing with a point Jean raised here, too.

Count me as opposed to this patch.

When I first looked at CodingStyle back in August, one thing that appealed
to me was the laid-back simpler style -- very few, very clear rules.

I'd prefer an addition to CodingStyle clarifying that we should not argue
about this minutiae.