[patch] checkpatch: putting the && or || on the wrong line

[patch] checkpatch: putting the && or || on the wrong line

[Dan Carpenter]

This patch makes checkpatch.pl complain if you break up conditions in
the wrong way.

Wrong:
	if ((really_long_condition)
		&& (second_condition)) { ...
Right:
	if ((really_long_condition) &&
		(second_condition)) { ...

If you do it in the wrong way the message is:  "put the && or || at the
end of the previous line"

Signed-off-by: Dan Carpenter <error27@gmail.com>

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index e3c7fc0..0a813db 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -1509,6 +1509,11 @@ sub process {
 			WARN("please, no space before tabs\n" . $herevet);
 		}
 
+# check for && or || at the start of a line
+		if ($rawline =~ /^\+\W+(&&|\|\|)/) {
+			WARN("put the && or || at the end of the previous line\n" . $herecurr);
+		}
+
 # check for spaces at the beginning of a line.
 # Exceptions:
 #  1) within comments

Re: [patch] checkpatch: putting the && or || on the wrong line

[Joe Perches]

On Tue, 2011-01-04 at 08:59 +0300, Dan Carpenter wrote:
> This patch makes checkpatch.pl complain if you break up conditions in
> the wrong way.
> 
> Wrong:
> 	if ((really_long_condition)
> 		&& (second_condition)) { ...
> Right:
> 	if ((really_long_condition) &&
> 		(second_condition)) { ...
> 
> If you do it in the wrong way the message is:  "put the && or || at the
> end of the previous line"
> 
> Signed-off-by: Dan Carpenter <error27@gmail.com>
> 
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index e3c7fc0..0a813db 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -1509,6 +1509,11 @@ sub process {
>  			WARN("please, no space before tabs\n" . $herevet);
>  		}
>  
> +# check for && or || at the start of a line
> +		if ($rawline =~ /^\+\W+(&&|\|\|)/) {

This should be /^\+[ \t]*(&&|\|\|)/

> +			WARN("put the && or || at the end of the previous line\n" . $herecurr);

I've submitted something like this a couple of times and gotten
various objections but I think it's sensible.

https://lkml.org/lkml/2009/12/5/65
https://lkml.org/lkml/2010/7/11/92

Re: [patch] checkpatch: putting the && or || on the wrong line

[Dan Carpenter]

On Mon, Jan 03, 2011 at 10:58:50PM -0800, Joe Perches wrote:
> On Tue, 2011-01-04 at 08:59 +0300, Dan Carpenter wrote:
> > +			WARN("put the && or || at the end of the previous line\n" . $herecurr);
> 
> I've submitted something like this a couple of times and gotten
> various objections but I think it's sensible.
> 
> https://lkml.org/lkml/2009/12/5/65
> https://lkml.org/lkml/2010/7/11/92

If everyone except you thinks it's a waste of time, then stop asking me
to redo my patches.  :/  I don't care either way so long as CodingStyle
and checkpatch reflect the rules.

regards,
dan carpenter

Re: [patch] checkpatch: putting the && or || on the wrong line

[Martin Knoblauch]

----- Original Message ----

> From: Dan Carpenter <error27@gmail.com>
> To: Joe Perches <joe@perches.com>
> Cc: Andy Whitcroft <apw@canonical.com>; linux-kernel@vger.kernel.org; 
>kernel-janitors@vger.kernel.org
> Sent: Tue, January 4, 2011 10:24:31 AM
> Subject: Re: [patch] checkpatch: putting the && or || on the wrong line
> 
> On Mon, Jan 03, 2011 at 10:58:50PM -0800, Joe Perches wrote:
> > On Tue,  2011-01-04 at 08:59 +0300, Dan Carpenter wrote:
> > > +             WARN("put the && or || at the end  of the previous line\n" . 
>$herecurr);
> > 
> > I've submitted something  like this a couple of times and gotten
> > various objections but I think  it's sensible.
> > 
> > https://lkml.org/lkml/2009/12/5/65
> > https://lkml.org/lkml/2010/7/11/92
> 
> If everyone except you thinks  it's a waste of time, then stop asking me
> to redo my patches.  :/   I don't care either way so long as CodingStyle
> and checkpatch reflect the  rules.
> 

 The problem is that CodingStyle (here and elsewhere) is viewed a the holy-grail 
or at least natural-law by some. It should not. It is just about style/taste and 
that will never suit everybody. Insisting to much on style is just driving those 
away that:

a) just want to contribute content
b1) have their own style for decades
b2) detest uniformity

 For *me* the sensible thing is to keep "style" when you make small 
modifications/patches to existing stuff, but use whatever style is best for you 
when contributing new stuff.

 Anyway, I just wanted to say "Happy New Year" to Linux :-)

Cheers
Martin

Re: [patch] checkpatch: putting the && or || on the wrong line

[J. Bruce Fields]

On Tue, Jan 04, 2011 at 08:59:00AM +0300, Dan Carpenter wrote:
> This patch makes checkpatch.pl complain if you break up conditions in
> the wrong way.
> 
> Wrong:
> 	if ((really_long_condition)
> 		&& (second_condition)) { ...
> Right:
> 	if ((really_long_condition) &&
> 		(second_condition)) { ...

As far as I can tell, the convention in mathematical typesetting is to
put operators on the left, not the right.  When the conditions are short
of there are more lines, that allows you to left-align on the repeated
operator.

--b.

> 
> If you do it in the wrong way the message is:  "put the && or || at the
> end of the previous line"
> 
> Signed-off-by: Dan Carpenter <error27@gmail.com>
> 
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index e3c7fc0..0a813db 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -1509,6 +1509,11 @@ sub process {
>  			WARN("please, no space before tabs\n" . $herevet);
>  		}
>  
> +# check for && or || at the start of a line
> +		if ($rawline =~ /^\+\W+(&&|\|\|)/) {
> +			WARN("put the && or || at the end of the previous line\n" . $herecurr);
> +		}
> +
>  # check for spaces at the beginning of a line.
>  # Exceptions:
>  #  1) within comments
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: [patch] checkpatch: putting the && or || on the wrong line

[Samuel Thibault]

J. Bruce Fields, le Tue 04 Jan 2011 11:38:36 -0500, a écrit :
> On Tue, Jan 04, 2011 at 08:59:00AM +0300, Dan Carpenter wrote:
> > This patch makes checkpatch.pl complain if you break up conditions in
> > the wrong way.
> > 
> > Wrong:
> > 	if ((really_long_condition)
> > 		&& (second_condition)) { ...
> > Right:
> > 	if ((really_long_condition) &&
> > 		(second_condition)) { ...
> 
> As far as I can tell, the convention in mathematical typesetting is to
> put operators on the left, not the right.  When the conditions are short
> of there are more lines, that allows you to left-align on the repeated
> operator.

I personnally find the left approach more readable.

Samuel
--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [patch] checkpatch: putting the && or || on the wrong line

[Joe Perches]

On Tue, 2011-01-04 at 17:44 +0100, Samuel Thibault wrote:
> J. Bruce Fields, le Tue 04 Jan 2011 11:38:36 -0500, a écrit :
> > On Tue, Jan 04, 2011 at 08:59:00AM +0300, Dan Carpenter wrote:
> > > This patch makes checkpatch.pl complain if you break up conditions in
> > > the wrong way.
> > > Wrong:
> > > 	if ((really_long_condition)
> > > 		&& (second_condition)) { ...
> > > Right:
> > > 	if ((really_long_condition) &&
> > > 		(second_condition)) { ...
> > As far as I can tell, the convention in mathematical typesetting is to
> > put operators on the left, not the right.  When the conditions are short
> > of there are more lines, that allows you to left-align on the repeated
> > operator.
> I personally find the left approach more readable.

As do I, but perhaps coding style in a project like this
shouldn't be personal but collective.

The trailing style outnumbers the leading style ~ 5:1.

$ grep -rP --include=*.[ch] "(\|\||&&)[ \t]*$" * | wc -l
39890
$ grep -rP --include=*.[ch] "^[ \t]*(\|\||&&)" * | wc -l
8244

If you take out drivers/staging, trailing is used ~ 6:1.

I think that high enough to be declared the preferred style.

--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [patch] checkpatch: putting the && or || on the wrong line

[Krzysztof Halasa]

Joe Perches <joe@perches.com> writes:

> As do I, but perhaps coding style in a project like this
> shouldn't be personal but collective.

I think there is nothing like a collective style.
What you can eventually achieve is a style everybody hates.

> The trailing style outnumbers the leading style ~ 5:1.
>
> $ grep -rP --include=*.[ch] "(\|\||&&)[ \t]*$" * | wc -l
> 39890
> $ grep -rP --include=*.[ch] "^[ \t]*(\|\||&&)" * | wc -l
> 8244
>
> If you take out drivers/staging, trailing is used ~ 6:1.
>
> I think that high enough to be declared the preferred style.

This is a very weak reason (if any at all) to do so. Increasing e.g.
readability of the code would be a good reason, but statistics?

Maybe: Microsoft Windows outnumbers Linux X:1, so it should be declared
the "preferred" system (= the only allowed, as with CodingStyle and
checkpatch "errors").

Or: cars outnumber trucks X:1, declare the trucks illegal.
Coffee drinkers outnumber tee drinkers, kill the later.


Yes, we need some basic common style (tabs length, unless/until we can
use any tab length), K&R (or other) parentheses, void *var instead of
void* var (void* var1, var2 bugs), (no) spaces etc. Anything less make
the code unreadable or less readable. We should stop dictating the
details when the benefits end, and they end pretty fast.
-- 
Krzysztof Halasa

Re: [patch] checkpatch: putting the && or || on the wrong line

[Joe Perches]

> Increasing e.g.
> readability of the code would be a good reason, but statistics?

Standardization of style in a large project increases
readability and decreases errors full stop.

Outside of that, it's _all_ bikeshedding.

cheers, Joe

Re: [patch] checkpatch: putting the && or || on the wrong line

[Martin Knoblauch]

----- Original Message ----

> From: Joe Perches <joe@perches.com>
> To: Krzysztof Halasa <khc@pm.waw.pl>
> Cc: Samuel Thibault <samuel.thibault@ens-lyon.org>; J. Bruce Fields 
><bfields@fieldses.org>; Dan Carpenter <error27@gmail.com>; Andy Whitcroft 
><apw@canonical.com>; linux-kernel@vger.kernel.org; 
>kernel-janitors@vger.kernel.org
> Sent: Wed, January 5, 2011 6:45:29 PM
> Subject: Re: [patch] checkpatch: putting the && or || on the wrong line
> 
> > Increasing e.g.
> > readability of the code would be a good reason, but  statistics?
> 
> Standardization of style in a large project  increases
> readability and decreases errors full stop.

 Anything that prevents errors from happening is good. No question.
 Anything that enforces "style" for no good reason is just counterproductive
 "readability" is a personal thing. It cannot be increased by enforcing "style". 
Not for everybody.

 And when discussing this issue we should not forget the environment. If I am a 
programmer working for a big$$$ company, I better follow their style-guide (if 
they have one). They pay my food and shelter after all. If I am "just" a 
volunteer on some mainly volunteer open source project, why should I care about 
"in my opinion" crap style. It will just drive me away from that project.

 Again as I wrote elsewhere - all depends on context. If a contribution is just 
a small patch to existing work one can easily follow the "style" of the existing 
work. If it is genuinely new work, it should be valued by the peers on technical 
merit, not on whether they like the contributors way of writing out an 
condition.

> 
> Outside of that,  it's _all_ bikeshedding.

 Your shed is my castle

> 
> cheers, Joe

Cheers
Martrin (to hell with style)

Re: [patch] checkpatch: putting the && or || on the wrong line

[Valdis.Kletnieks]

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

On Thu, 06 Jan 2011 04:11:31 PST, Martin Knoblauch said:
> If I am "just" a volunteer on some mainly volunteer open source project, why
> should I care about "in my opinion" crap style. It will just drive me away from
> that project.

> Martrin (to hell with style)

http://www.ioccc.org/ is over there --------------------------->

That's why you should care about style - because nobody likes debugging
somebody else's ioccc award-winning code. ;)

[-- Attachment #2: Type: application/pgp-signature, Size: 227 bytes --]

Re: [patch] checkpatch: putting the && or || on the wrong line

[Krzysztof Halasa]

Joe Perches <joe@perches.com> writes:

> Standardization of style in a large project increases
> readability and decreases errors full stop.

You forgot to attach the evidence.

Sure, standardization is a good thing - to a certain point. We've passed
this point long ago.
-- 
Krzysztof Halasa

Re: [patch] checkpatch: putting the && or || on the wrong line

[Joe Perches]

On Thu, 2011-01-06 at 13:32 +0100, Krzysztof Halasa wrote:
> Sure, standardization is a good thing - to a certain point.
> We've passed this point long ago.

Aren't you missing attached evidence too? :p

cheers, Joe

Re: [patch] checkpatch: putting the && or || on the wrong line

[Krzysztof Halasa]

Joe Perches <joe@perches.com> writes:

>> Sure, standardization is a good thing - to a certain point.
>> We've passed this point long ago.
>
> Aren't you missing attached evidence too? :p

No, it's all over the place.
-- 
Krzysztof Halasa

Re: [patch] checkpatch: putting the && or || on the wrong line

[Joe Perches]

On Thu, 2011-01-06 at 21:23 +0100, Krzysztof Halasa wrote:
> Joe Perches <joe@perches.com> writes:
> >> Sure, standardization is a good thing - to a certain point.
> >> We've passed this point long ago.
> > Aren't you missing attached evidence too? :p
> No, it's all over the place.

Right.  It's all exactly the same.
We agree.  Cites aren't necessary.

cheers, Joe

Re: [patch] checkpatch: putting the && or || on the wrong line

[Krzysztof Halasa]

Joe Perches <joe@perches.com> writes:

>> >> Sure, standardization is a good thing - to a certain point.
>> >> We've passed this point long ago.

> Right.  It's all exactly the same.
> We agree.  Cites aren't necessary.

People complained on lkml and other lists that the CodingStyle /
checkpatch went way too far many times. So the evidence is there, in the
list archives, and I guess even now you're getting feedback on this.

OTOH you failed to show evidence that super-strict standardization
benefits anyone.
-- 
Krzysztof Halasa

Re: [patch] checkpatch: putting the && or || on the wrong line

[Joe Perches]

On Thu, 2011-01-06 at 22:14 +0100, Krzysztof Halasa wrote:
> Joe Perches <joe@perches.com> writes:
> >> >> Sure, standardization is a good thing - to a certain point.
> >> >> We've passed this point long ago.
> > Right.  It's all exactly the same.
> > We agree.  Cites aren't necessary.
> People complained on lkml and other lists that the CodingStyle /
> checkpatch went way too far many times. So the evidence is there, in the
> list archives, and I guess even now you're getting feedback on this.

People complain, that's a fact.

> OTOH you failed to show evidence that super-strict standardization
> benefits anyone.

I don't need to.

If you don't agree with the assertion,
facts likely won't change your mind.
You'll more likely dispute the facts.

Look up this paper if you care to though:

Evaluating the Relation Between Coding
Standard Violations and Faults Within and
Across Software Versions

Cathal Boogerd and Leon Moonen

http://swerl.tudelft.nl/twiki/pub/Main/TechnicalReports/TUD-SERG-2009-008.pdf

RQ2 Are files or modules with a higher violation
density more fault-prone?

This holds for 10 rules in the standard, with some reserva-
tions. There is no reliable prediction for files without ac-
tive development (no changes) nor for files without viola-
tions. Also, the observed relation becomes less pronounced
in time, as the number of registered open faults decreases.

Re: [patch] checkpatch: putting the && or || on the wrong line

[Valdis.Kletnieks]

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

On Thu, 06 Jan 2011 13:38:38 PST, Joe Perches said:

> http://swerl.tudelft.nl/twiki/pub/Main/TechnicalReports/TUD-SERG-2009-008.pdf
> 
> RQ2 Are files or modules with a higher violation
> density more fault-prone?
> 
> This holds for 10 rules in the standard, with some reserva-
> tions. There is no reliable prediction for files without ac-
> tive development (no changes) nor for files without viola-
> tions. Also, the observed relation becomes less pronounced
> in time, as the number of registered open faults decreases.

In other words, code not written to style is like finding a brown M&M
at a Van Halen concert...

[-- Attachment #2: Type: application/pgp-signature, Size: 227 bytes --]

Re: [patch] checkpatch: putting the && or || on the wrong line

[Krzysztof Halasa]

Joe Perches <joe@perches.com> writes:

>> OTOH you failed to show evidence that super-strict standardization
>> benefits anyone.
>
> I don't need to.
>
> If you don't agree with the assertion,
> facts likely won't change your mind.
> You'll more likely dispute the facts.

Sounds like you have the hard facts. Why not let us know anyway?

> Look up this paper if you care to though:
>
> Evaluating the Relation Between Coding
> Standard Violations and Faults Within and
> Across Software Versions

This is apples vs oranges, nobody disputes a need for sensible rules.
-- 
Krzysztof Halasa

Re: [patch] checkpatch: putting the && or || on the wrong line

[Joe Perches]

On Sat, 2011-01-08 at 14:42 +0100, Krzysztof Halasa wrote:
> Joe Perches <joe@perches.com> writes:
> >> OTOH you failed to show evidence that super-strict standardization
> >> benefits anyone.
> > I don't need to.
> > If you don't agree with the assertion,
> > facts likely won't change your mind.
> > You'll more likely dispute the facts.
> Sounds like you have the hard facts. Why not let us know anyway?
> > Look up this paper if you care to though:
> > Evaluating the Relation Between Coding
> > Standard Violations and Faults Within and
> > Across Software Versions
> This is apples vs oranges, nobody disputes a need for sensible rules.

Classic example of the disutility of fact based argumentation.

Krzyzstof, let's chat about something like the weather next time.

cheers, Joe

Re: [patch] checkpatch: putting the && or || on the wrong line

[Martin Knoblauch]

----- Original Message ----

> From: Krzysztof Halasa <khc@pm.waw.pl>
> To: Joe Perches <joe@perches.com>
> Cc: Samuel Thibault <samuel.thibault@ens-lyon.org>; J. Bruce Fields 
><bfields@fieldses.org>; Dan Carpenter <error27@gmail.com>; Andy Whitcroft 
><apw@canonical.com>; linux-kernel@vger.kernel.org; 
>kernel-janitors@vger.kernel.org
> Sent: Wed, January 5, 2011 6:38:07 PM
> Subject: Re: [patch] checkpatch: putting the && or || on the wrong line
> 
> Joe Perches <joe@perches.com> writes:
> 
> > As do  I, but perhaps coding style in a project like this
> > shouldn't be personal  but collective.
> 
> I think there is nothing like a collective style.
> What  you can eventually achieve is a style everybody hates.
> 
> > The trailing  style outnumbers the leading style ~ 5:1.
> >
> > $ grep -rP  --include=*.[ch] "(\|\||&&)[ \t]*$" * | wc -l
> > 39890
> > $  grep -rP --include=*.[ch] "^[ \t]*(\|\||&&)" * | wc -l
> >  8244
> >
> > If you take out drivers/staging, trailing is used ~  6:1.
> >
> > I think that high enough to be declared the preferred  style.

 Nobody defines which style *I* prefer. One (in this case the compiler) may 
define which laws I obey, but not which style I like. As long as the compiler 
accepts both notations and generates the same code there is no "law" against 
either.

> 
> This is a very weak reason (if any at all) to do so. Increasing  e.g.

 indeed.

> readability of the code would be a good reason, but  statistics?
> 

 But who is defining readability? That is not a technical term at all. What I 
view as readable may completely from your or Joes or anybody elses opinion.

> Maybe: Microsoft Windows outnumbers Linux X:1, so it should  be declared
> the "preferred" system (= the only allowed, as with CodingStyle  and
> checkpatch "errors").
> 
> Or: cars outnumber trucks X:1, declare the  trucks illegal.

 I would really think about supporting that :-)

> Coffee drinkers outnumber tee drinkers, kill the  later.
>

 Here I am for personal freedom :-)
 
> 
> Yes, we need some basic common style (tabs length,  unless/until we can
> use any tab length), K&R (or other) parentheses, void  *var instead of
> void* var (void* var1, var2 bugs), (no) spaces etc. Anything  less make

 Actually if this allows ambigous code, the language has a problem. But yeah, a 
tool to enforce one way is good.

> the code unreadable or less readable. We should stop dictating  the
> details when the benefits end, and they end pretty fast.

 Amen.

Cheers
Martin

Re: [patch] checkpatch: putting the && or || on the wrong line

[Krzysztof Halasa]

Martin Knoblauch <spamtrap@knobisoft.de> writes:

>  But who is defining readability? That is not a technical term at all. What I 
> view as readable may completely from your or Joes or anybody elses
> opinion.

Readability as perceived by the majority of readers. This is where the
whole community (as opposed to single given individuals) benefits.
-- 
Krzysztof Halasa