Skip to contentSkip to conversation

checkpatch: putting the && or || on the wrong line

PATCH In review Dan Carpenter 8 participants 22 messages Jan 4, 2011 – Jan 8, 2011

Subsystems (2)
Kernel janitors Checkpatch
lore
Share & tools
lore

Dan Carpenter

author
#0 lore raw

PATCH 1/1 checkpatch: putting the && or || on the wrong line

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 <[email protected]>
scripts/checkpatch.pl +5 -0
index e3c7fc0..0a813db 100755
@@ -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
maintainer
#1 lore raw
Quoted 20 lines · 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 <[email protected]>
scripts/checkpatch.pl +0 -0
index e3c7fc0..0a813db 100755

scripts/checkpatch.pl sub process { · line 1513

Quoted 5 lines
scripts/checkpatch.pl +2 -0
@@ -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]*(&&|\|\|)/

scripts/checkpatch.pl sub process { · line 1514

Quoted 1 line
scripts/checkpatch.pl +1 -0
+			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

author
#2 lore raw
Quoted 8 lines · Joe Perches

scripts/checkpatch.pl sub process { · line 1514

Quoted 1 line · Dan Carpenter
scripts/checkpatch.pl +1 -0
+			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

#3 lore raw

----- Original Message ----

Quoted 15 lines

From: Dan Carpenter <[email protected]>
To: Joe Perches <[email protected]>
Cc: Andy Whitcroft <[email protected]>; [email protected]; [email protected]
Sent: Tue, January 4, 2011 10:24:31 AM
Subject: Re: [patch] checkpatch: putting the && or || on the wrong line

Quoted 2 lines · Joe Perches
Quoted 1 line · Dan Carpenter

+ WARN("put the && or || at the end of the previous line\n" .

$herecurr);

Quoted 4 lines

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

Quoted 7 lines
Quoted 2 lines

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

#4 lore raw
In reply to Dan Carpenter
Quoted 9 lines · 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)) { ...

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.

Quoted 27 lines

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 <[email protected]>
scripts/checkpatch.pl +5 -1
index e3c7fc0..0a813db 100755
@@ -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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

#5 lore raw
Quoted 11 lines · J. Bruce Fields, le Tue 04 Jan 2011 11:38:36 -0500, a écrit :
Quoted 9 lines · 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)) { ...
Quoted 4 lines

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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

maintainer
#6 lore raw
Quoted 10 lines · Samuel Thibault

J. Bruce Fields, le Tue 04 Jan 2011 11:38:36 -0500, a écrit :

Quoted 9 lines · Dan Carpenter
Quoted 8 lines

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)) { ...
Quoted 5 lines
Quoted 4 lines

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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

#7 lore raw
Quoted 2 lines · Joe Perches

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.

Quoted 10 lines

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.

Signature · 1 line
Krzysztof Halasa
maintainer
#8 lore raw
Quoted 2 lines

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

#9 lore raw

----- Original Message ----

Quoted 9 lines

From: Joe Perches <[email protected]>
To: Krzysztof Halasa <[email protected]>
Cc: Samuel Thibault <[email protected]>; J. Bruce Fields <[email protected]>; Dan Carpenter <[email protected]>; Andy Whitcroft <[email protected]>; [email protected]; [email protected]
Sent: Wed, January 5, 2011 6:45:29 PM
Subject: Re: [patch] checkpatch: putting the && or || on the wrong line

Quoted 5 lines
Quoted 2 lines

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.

Quoted 2 lines

Outside of that, it's _all_ bikeshedding.

Your shed is my castle

Quoted 2 lines

cheers, Joe

Cheers
Martrin (to hell with style)

#10 lore raw
Quoted 5 lines · Martin Knoblauch

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. ;)

#11 lore raw
In reply to Joe Perches
Quoted 2 lines · Joe Perches

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.

Signature · 1 line
Krzysztof Halasa
maintainer
#12 lore raw
Quoted 2 lines · Krzysztof Halasa

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

#13 lore raw
Quoted 4 lines · Joe Perches
Quoted 2 lines

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.

Signature · 1 line
Krzysztof Halasa
maintainer
#14 lore raw
Quoted 5 lines · Krzysztof Halasa
Quoted 3 lines · Joe Perches
Quoted 2 lines

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

#15 lore raw
Quoted 5 lines · Joe Perches
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines

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.

Signature · 1 line
Krzysztof Halasa
maintainer
#16 lore raw
Quoted 8 lines · Krzysztof Halasa
Quoted 4 lines · Joe Perches
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines

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.

Quoted 2 lines

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.

#17 lore raw
Quoted 10 lines · Joe Perches

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

#18 lore raw
In reply to Joe Perches
Quoted 8 lines · Joe Perches
Quoted 2 lines

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?

Quoted 5 lines

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.

Signature · 1 line
Krzysztof Halasa
maintainer
#19 lore raw
Quoted 7 lines · Krzysztof Halasa
Quoted 6 lines · Joe Perches
Quoted 2 lines

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.

Quoted 6 lines

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

Quoted 4 lines

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

#20 lore raw
In reply to Krzysztof Halasa

----- Original Message ----

Quoted 9 lines

From: Krzysztof Halasa <[email protected]>
To: Joe Perches <[email protected]>
Cc: Samuel Thibault <[email protected]>; J. Bruce Fields <[email protected]>; Dan Carpenter <[email protected]>; Andy Whitcroft <[email protected]>; [email protected]; [email protected]
Sent: Wed, January 5, 2011 6:38:07 PM
Subject: Re: [patch] checkpatch: putting the && or || on the wrong line

Quoted 18 lines
Quoted 2 lines · Joe Perches

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.

Quoted 10 lines

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.

Quoted 2 lines

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

indeed.

Quoted 2 lines

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.

Quoted 5 lines

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 :-)

Quoted 2 lines

Coffee drinkers outnumber tee drinkers, kill the later.

Here I am for personal freedom :-)

Quoted 4 lines

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.

Quoted 2 lines

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

Amen.

Cheers
Martin

#21 lore raw
Quoted 3 lines · Martin Knoblauch

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.

Signature · 1 line
Krzysztof Halasa