[PATCH] checkpatch: Add --strict tests for braces, comments and casts

[PATCH] checkpatch: Add --strict tests for braces, comments and casts

[Joe Perches]

Add some more subjective --strict tests.

Add a test for block comments that start with a blank
line followed only by a line with just the comment block
initiator.  Prefer a blank line followed by /* comment...

Add a test for unnecessary spaces after a cast.

Add a test for symmetric uses of braces in if/else blocks.
If one branch needs braces, then all branches should use braces.

Signed-off-by: Joe Perches <joe@perches.com>
---
 scripts/checkpatch.pl |   40 ++++++++++++++++++++++++++++++++--------
 1 files changed, 32 insertions(+), 8 deletions(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 308e401..581a14c 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -1849,6 +1849,17 @@ sub process {
 			}
 		}
 
+		if ($line =~ /^\+.*\*[ \t]*\)[ \t]+/) {
+			CHK("SPACING",
+			    "No space is necessary after a cast\n" . $hereprev);
+		}
+
+		if ($rawline =~ /^\+[ \t]*\/\*[ \t]*$/ &&
+		    $prevrawline =~ /^\+[ \t]*$/) {
+			CHK("BLOCK_COMMENT_STYLE",
+			    "Don't begin block comments with only a /* line, use /* comment...\n" . $hereprev);
+		}
+
 # check for spaces at the beginning of a line.
 # Exceptions:
 #  1) within comments
@@ -2954,7 +2965,8 @@ sub process {
 			#print "chunks<$#chunks> linenr<$linenr> endln<$endln> level<$level>\n";
 			#print "APW: <<$chunks[1][0]>><<$chunks[1][1]>>\n";
 			if ($#chunks > 0 && $level == 0) {
-				my $allowed = 0;
+				my @allowed = ();
+				my $allow = 0;
 				my $seen = 0;
 				my $herectx = $here . "\n";
 				my $ln = $linenr - 1;
@@ -2965,6 +2977,7 @@ sub process {
 					my ($whitespace) = ($cond =~ /^((?:\s*\n[+-])*\s*)/s);
 					my $offset = statement_rawlines($whitespace) - 1;
 
+					$allowed[$allow] = 0;
 					#print "COND<$cond> whitespace<$whitespace> offset<$offset>\n";
 
 					# We have looked at and allowed this specific line.
@@ -2977,23 +2990,34 @@ sub process {
 
 					$seen++ if ($block =~ /^\s*{/);
 
-					#print "cond<$cond> block<$block> allowed<$allowed>\n";
+					#print "cond<$cond> block<$block> allowed<$allowed[$allow]>\n";
 					if (statement_lines($cond) > 1) {
 						#print "APW: ALLOWED: cond<$cond>\n";
-						$allowed = 1;
+						$allowed[$allow] = 1;
 					}
 					if ($block =~/\b(?:if|for|while)\b/) {
 						#print "APW: ALLOWED: block<$block>\n";
-						$allowed = 1;
+						$allowed[$allow] = 1;
 					}
 					if (statement_block_size($block) > 1) {
 						#print "APW: ALLOWED: lines block<$block>\n";
-						$allowed = 1;
+						$allowed[$allow] = 1;
 					}
+					$allow++;
 				}
-				if ($seen && !$allowed) {
-					WARN("BRACES",
-					     "braces {} are not necessary for any arm of this statement\n" . $herectx);
+				if ($seen) {
+					my $sum_allowed = 0;
+					foreach (@allowed) {
+						$sum_allowed += $_;
+					}
+					if ($sum_allowed == 0) {
+						WARN("BRACES",
+						     "braces {} are not necessary for any arm of this statement\n" . $herectx);
+					} elsif ($sum_allowed != $allow &&
+						 $seen != $allow) {
+						CHK("BRACES",
+						    "braces {} should be used on all arms of this statement\n" . $herectx);
+					}
 				}
 			}
 		}
-- 
1.7.8.111.gad25c.dirty

[PATCH] checkpatch: Add --strict test for strings split across multiple lines

[Joe Perches]

Strings split across multiple lines are commonly used
as formats.  These uncoalesced formats are hard to grep
and are relatively error prone.

Suggest coalescing split strings when using --strict.

Signed-off-by: Joe Perches <joe@perches.com>
---
 scripts/checkpatch.pl |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 581a14c..5d0b46c 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -1860,6 +1860,12 @@ sub process {
 			    "Don't begin block comments with only a /* line, use /* comment...\n" . $hereprev);
 		}
 
+		if ($prevline =~ /^\+.*"[ \t]*$/ &&
+		    $line =~ /^\+[ \t]*"/) {
+			CHK("COALESCE_STRING",
+			    "Coalesced strings are easier to grep and less error prone\n" . $hereprev);
+		}
+
 # check for spaces at the beginning of a line.
 # Exceptions:
 #  1) within comments
-- 
1.7.8.111.gad25c.dirty

[PATCH] checkpatch: Suggest pr_<level> over printk(KERN_<LEVEL>

[Joe Perches]

Suggest the shorter pr_<level> instead of printk(KERN_<LEVEL>.

Prefer to use pr_<level> over bare printks.
Prefer to use pr_warn over pr_warning.

Signed-off-by: Joe Perches <joe@perches.com>
---
 scripts/checkpatch.pl |   13 +++++++++++++
 1 files changed, 13 insertions(+), 0 deletions(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 04eb9eb..ac2bbea 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -2379,6 +2379,19 @@ sub process {
 			}
 		}
 
+		if ($line =~ /\bprintk\s*\(\s*KERN_([A-Z]+)/) {
+			my $orig = $1;
+			my $level = lc($orig);
+			$level = "warn" if ($level eq "warning");
+			WARN("PREFER_PR_LEVEL",
+			     "Prefer pr_$level(... to printk(KERN_$1, ...\n" . $herecurr);
+		}
+
+		if ($line =~ /\bpr_warning\s*\(/) {
+			WARN("PREFER_PR_LEVEL",
+			     "Prefer pr_warn(... to pr_warning(...\n" . $herecurr);
+		}
+
 # function brace can't be on same line, except for #defines of do while,
 # or if closed on same line
 		if (($line=~/$Type\s*$Ident\(.*\).*\s{/) and

Re: [PATCH] checkpatch: Suggest pr_<level> over printk(KERN_<LEVEL>

[Ted Ts'o]

On Mon, Mar 12, 2012 at 11:23:03PM -0700, Joe Perches wrote:
> Suggest the shorter pr_<level> instead of printk(KERN_<LEVEL>.
> 
> Prefer to use pr_<level> over bare printks.
> Prefer to use pr_warn over pr_warning.
> 
> Signed-off-by: Joe Perches <joe@perches.com>

Is this even worth a warning?   I don't think so....

   	     	     		  	- Ted

Re: [PATCH] checkpatch: Suggest pr_<level> over printk(KERN_<LEVEL>

[Andrew Morton]

On Tue, 13 Mar 2012 08:05:14 -0400
"Ted Ts'o" <tytso@mit.edu> wrote:

> On Mon, Mar 12, 2012 at 11:23:03PM -0700, Joe Perches wrote:
> > Suggest the shorter pr_<level> instead of printk(KERN_<LEVEL>.
> > 
> > Prefer to use pr_<level> over bare printks.
> > Prefer to use pr_warn over pr_warning.
> > 
> > Signed-off-by: Joe Perches <joe@perches.com>
> 
> Is this even worth a warning?   I don't think so....

mm...  probably.  It's not a thing I ever bother mentioning in review,
but I guess pr_foo() is a bit denser, and doing the same thing in two
different ways is always an irritant.

I'll put the patch in my tree for a while and see how irritating I find
it ;)

Re: [PATCH] checkpatch: Suggest pr_<level> over printk(KERN_<LEVEL>

[Ted Ts'o]

On Tue, Mar 13, 2012 at 02:55:17PM -0700, Andrew Morton wrote:
> 
> mm...  probably.  It's not a thing I ever bother mentioning in review,
> but I guess pr_foo() is a bit denser, and doing the same thing in two
> different ways is always an irritant.

Sure but if a particular kernel file or subsystem is _not_ using
pr_foo(), having a checkpatch which tries to force everyone to use
pr_foo() is going to be really annoying to me as a maintainer...

	    	     	       		- Ted

Re: [PATCH] checkpatch: Suggest pr_<level> over printk(KERN_<LEVEL>

[Andrew Morton]

On Tue, 13 Mar 2012 18:01:44 -0400
"Ted Ts'o" <tytso@mit.edu> wrote:

> On Tue, Mar 13, 2012 at 02:55:17PM -0700, Andrew Morton wrote:
> > 
> > mm...  probably.  It's not a thing I ever bother mentioning in review,
> > but I guess pr_foo() is a bit denser, and doing the same thing in two
> > different ways is always an irritant.
> 
> Sure but if a particular kernel file or subsystem is _not_ using
> pr_foo(), having a checkpatch which tries to force everyone to use
> pr_foo() is going to be really annoying to me as a maintainer...
> 

Yes, that's what I fear.  That's why I'm testing it...

Re: [PATCH] checkpatch: Suggest pr_<level> over printk(KERN_<LEVEL>

[Ted Ts'o]

On Tue, Mar 13, 2012 at 03:03:16PM -0700, Andrew Morton wrote:
> > Sure but if a particular kernel file or subsystem is _not_ using
> > pr_foo(), having a checkpatch which tries to force everyone to use
> > pr_foo() is going to be really annoying to me as a maintainer...
> 
> Yes, that's what I fear.  That's why I'm testing it...

Perhaps the answer is a ".checkpatch_ignore" file that can be
maintained by maintainers in each directory to suppress specific
checkpatch warnings that the maintainers don't agree with.  That way
we don't have to worry about checkpatch maintainers try to impose
their pet programming style preferences on maintainers who don't want
to get random trivial style-fixing patches from well-meaning kernel
newbies...

						- Ted

Re: [PATCH] checkpatch: Suggest pr_<level> over printk(KERN_<LEVEL>

[Joe Perches]

On Tue, 2012-03-13 at 20:31 -0400, Ted Ts'o wrote:
> On Tue, Mar 13, 2012 at 03:03:16PM -0700, Andrew Morton wrote:
> > > Sure but if a particular kernel file or subsystem is _not_ using
> > > pr_foo(), having a checkpatch which tries to force everyone to use
> > > pr_foo() is going to be really annoying to me as a maintainer...
> > 
> > Yes, that's what I fear.  That's why I'm testing it...
> 
> Perhaps the answer is a ".checkpatch_ignore" file that can be
> maintained by maintainers in each directory to suppress specific
> checkpatch warnings that the maintainers don't agree with.  That way
> we don't have to worry about checkpatch maintainers try to impose
> their pet programming style preferences on maintainers who don't want
> to get random trivial style-fixing patches from well-meaning kernel
> newbies...

Or add an I: line to MAINTAINERS

though perhaps it's better to agree on a style.

I did send a few fixes and a style consolidation patch
for ext4 with no reply awhile ago:

https://lkml.org/lkml/2011/8/2/41

Re: [PATCH] checkpatch: Suggest pr_<level> over printk(KERN_<LEVEL>

[Ted Ts'o]

On Tue, Mar 13, 2012 at 05:47:06PM -0700, Joe Perches wrote:
> Or add an I: line to MAINTAINERS
> 
> though perhaps it's better to agree on a style.
> 
> I did send a few fixes and a style consolidation patch
> for ext4 with no reply awhile ago:
> 
> https://lkml.org/lkml/2011/8/2/41

You combined a huge number of changes into a single patch, and as far
as I was concerned the value it added Just Wasn't Worth It.  It adds
noise which causes other patches, which add real value, not to apply
cleanly.

I routinely ignore such patches because I have a limited amount of
time, and as far as I'm concerned they mainly make my life harder.

Looking more closely, there are a few changes in there that I'd
accept, but it was burried amongst so much other junk that if it's all
or nothing, it would be nothing.  Funny that someone who is an expert
in style things neglected something **far** more important ---
segregate logical changes in separate commits; don't collapse
everything into a single patch, which makes it hard to review.

      	     	    		  - Ted

Re: [PATCH] checkpatch: Suggest pr_<level> over printk(KERN_<LEVEL>

[Joe Perches]

On Tue, 2012-03-13 at 21:07 -0400, Ted Ts'o wrote:
> On Tue, Mar 13, 2012 at 05:47:06PM -0700, Joe Perches wrote:
> > Or add an I: line to MAINTAINERS
> > 
> > though perhaps it's better to agree on a style.
> > 
> > I did send a few fixes and a style consolidation patch
> > for ext4 with no reply awhile ago:
> > 
> > https://lkml.org/lkml/2011/8/2/41
> 
> You combined a huge number of changes into a single patch, and as far
> as I was concerned the value it added Just Wasn't Worth It.  It adds
> noise which causes other patches, which add real value, not to apply
> cleanly.
> 
> I routinely ignore such patches because I have a limited amount of
> time, and as far as I'm concerned they mainly make my life harder.
> 
> Looking more closely, there are a few changes in there that I'd
> accept, but it was burried amongst so much other junk that if it's all
> or nothing, it would be nothing.  Funny that someone who is an expert
> in style things neglected something **far** more important ---
> segregate logical changes in separate commits; don't collapse
> everything into a single patch, which makes it hard to review.

The patch was all apiece, every bit associated to logging output.
It was bundled to make it easier to apply.

You call it junk, I call it cleanups.

Consistent style in a largish project has advantages.
You can ignore them if you choose.

Re: [PATCH] checkpatch: Suggest pr_<level> over printk(KERN_<LEVEL>

[Ted Ts'o]

On Tue, Mar 13, 2012 at 06:17:11PM -0700, Joe Perches wrote:
> The patch was all apiece, every bit associated to logging output.
> It was bundled to make it easier to apply.
> 
> You call it junk, I call it cleanups.

Changing stuff to pr_foo is ***NOISE***.  It adds no value, and it
makes it my life much, MUCH harder.

Andrew, please, can we NACK this pr_foo checkpatch.pl change?

							- Ted

Re: [PATCH] checkpatch: Suggest pr_<level> over printk(KERN_<LEVEL>

[Joe Perches]

On Tue, 2012-03-13 at 22:19 -0400, Ted Ts'o wrote:
> On Tue, Mar 13, 2012 at 06:17:11PM -0700, Joe Perches wrote:
> > The patch was all apiece, every bit associated to logging output.
> > It was bundled to make it easier to apply.
> > 
> > You call it junk, I call it cleanups.
> 
> Changing stuff to pr_foo is ***NOISE***.  It adds no value, and it
> makes it my life much, MUCH harder.

I dispute that it add no value.

pr_<foo> adds value to the dmesg output because
it can be consistently prefixed via pr_fmt.

Right now many fs ext4 messages are somewhat opaque
without any reference to what kernel subsystem produced
the message.

For instance:

fs/ext4/ialloc.c:		printk(KERN_DEBUG "group %lu: stored = %d, counted = %lu\n",

This is a somewhat senseless output in dmesg without
any linkage to ext4.

Using pr_fmt and pr_debug as I sent a patch to do
instead emits in dmesg:

EXT4-fs: group: etc...

Using subsystem prefixes makes it easy and consistent to
grep dmesg.

cheers, Joe

Re: [PATCH] checkpatch: Suggest pr_<level> over printk(KERN_<LEVEL>

[Ted Ts'o]

On Tue, Mar 13, 2012 at 07:31:51PM -0700, Joe Perches wrote:
> Right now many fs ext4 messages are somewhat opaque
> without any reference to what kernel subsystem produced
> the message.
> 
> For instance:
> 
> fs/ext4/ialloc.c:		printk(KERN_DEBUG "group %lu: stored = %d, counted = %lu\n",
> 
> This is a somewhat senseless output in dmesg without
> any linkage to ext4.
> 
> Using pr_fmt and pr_debug as I sent a patch to do
> instead emits in dmesg:
> 
> EXT4-fs: group: etc...
> 
> Using subsystem prefixes makes it easy and consistent to
> grep dmesg.

That's a debug message which is never by anyone other than ext4
developers.  Your patch also hacked the Makefile to enable it by
default, which also enabled some performance degrading code paths
(again, only enabled by developers who manually drop the #define in a
header file when they are trying to figure out some obscure failure
during the development process).  This is why I don't like people who
are wanking around in code they don't understand just to fix style
fixes, in the mistaken belief that it adds value.

						- Ted

Re: [PATCH] checkpatch: Suggest pr_<level> over printk(KERN_<LEVEL>

[Joe Perches]

On Tue, 2012-03-13 at 22:41 -0400, Ted Ts'o wrote:
> On Tue, Mar 13, 2012 at 07:31:51PM -0700, Joe Perches wrote:
> > Right now many fs ext4 messages are somewhat opaque
> > without any reference to what kernel subsystem produced
> > the message.
> > 
> > For instance:
> > 
> > fs/ext4/ialloc.c:		printk(KERN_DEBUG "group %lu: stored = %d, counted = %lu\n",
> > 
> > This is a somewhat senseless output in dmesg without
> > any linkage to ext4.
> > 
> > Using pr_fmt and pr_debug as I sent a patch to do
> > instead emits in dmesg:
> > 
> > EXT4-fs: group: etc...
> > 
> > Using subsystem prefixes makes it easy and consistent to
> > grep dmesg.
> 
> That's a debug message which is never by anyone other than ext4
> developers.  Your patch also hacked the Makefile to enable it by
> default,

It's just an example and no it didn't.
That output is still in an #ifdef EXT4FS_DEBUG
block and is unchanged.

What I did was #define DEBUG so pr_debug
(and so dynamic_debug if desired as well)
emits output to dmesg.

+ccflags-$(CONFIG_EXT4_FS) := -DDEBUG

ext4 doesn't currently use any #ifdef DEBUG blocks.

> which also enabled some performance degrading code paths
> (again, only enabled by developers who manually drop the #define in a
> header file when they are trying to figure out some obscure failure
> during the development process).  This is why I don't like people who
> are wanking around in code they don't understand just to fix style
> fixes, in the mistaken belief that it adds value.

cheers, Joe

Re: [PATCH] checkpatch: Suggest pr_<level> over printk(KERN_<LEVEL>

[Ted Ts'o]

On Tue, Mar 13, 2012 at 08:01:14PM -0700, Joe Perches wrote:
> > That's a debug message which is never by anyone other than ext4
> > developers.  Your patch also hacked the Makefile to enable it by
> > default,
> 
> It's just an example and no it didn't.
> That output is still in an #ifdef EXT4FS_DEBUG
> block and is unchanged.

I looked at your patch, and nearly all of them were in debug code.  I
know, because in practice the messages that come up with any kind of
regularity are all properly prefixed.

Look, the reason why I care about patch noise is because I have a huge
patch backlog.  Take a look at this:

      http://patchwork.ozlabs.org/project/linux-ext4/list/

Very few other people review patches, and even patches that survive
review, I've found problems that could potentially lead to data loss
or system instability.  This is not like your average device driver,
where if the machine panics once a week, "oh well", and you reboot.
Linus would get very cranky if he lost data as a result of a bad patch
slipping through.  Hence, patches don't go in until after significant
review and testing.

As a result, #1, patches that are don't add value, and are large,
simply won't get applied.  Period.  Avoiding the downside of lots of
people losing data is ****far**** more than your OCD wanting me to use
pr_warn(...) instead of printk(KERN_WARN, ...).

If you want your style patches to go in, break them into smaller
chunks, or I *will* ignore them.

#2, patches that don't add substantial value, and make it harder for
me to review and apply patches from my very substantial patch
backlog, I consider as adding ***substantial*** negative value.

That being said, I use checkpatch.pl as an initial screen, but it's
already been the case that there are warnings that I consider pure
noise, and I really, REALLY, rather not add more noise to
checkpatch.pl.  There is no group consensus about things like pr_foo
--- not as far as I've seen --- and to impose it from on high by some
patch wankers making changes to checkpatch.pl very much offends me.

      	      	     	     		      	   - Ted

Re: [PATCH] checkpatch: Suggest pr_<level> over printk(KERN_<LEVEL>

[Joe Perches]

On Wed, 2012-03-14 at 08:34 -0400, Ted Ts'o wrote:
> On Tue, Mar 13, 2012 at 08:01:14PM -0700, Joe Perches wrote:
> > > That's a debug message which is never by anyone other than ext4
> > > developers.  Your patch also hacked the Makefile to enable it by
> > > default,
> > 
> > It's just an example and no it didn't.
> > That output is still in an #ifdef EXT4FS_DEBUG
> > block and is unchanged.
> 
> I looked at your patch, and nearly all of them were in debug code.  I
> know, because in practice the messages that come up with any kind of
> regularity are all properly prefixed.

Not really, some are prefixed with EXT4-fs, others EXT4,
some with colons, some without, some with no prefix,
some with function names only.

The idea is to be consistent and allow a mechanical
comprehensive dmesg grep with "EXT4-fs:" or some
other appropriate subsystem name.

$ grep -rP --include=*.[ch] "\bprintk\s*\(\s*KERN_[A-Z]+\s*\"[^\":]*" fs/ext4/

>       http://patchwork.ozlabs.org/project/linux-ext4/list/

Patchwork queues are pretty useless when patches entered
do not have their status updated for long periods.

The patch I sent in August 2011 shows "new" rather than
have an appropriate status.

There are patches in that queue from 2008 marked as "new"
that will never be applied or looked at again.

If you actually use patchwork, though it seems you don't,
I think you should just mark every patch that's new as
rejected and start over.

> Very few other people review patches, and even patches that survive
> review, I've found problems that could potentially lead to data loss
> or system instability.  This is not like your average device driver,
> where if the machine panics once a week, "oh well", and you reboot.
> Linus would get very cranky if he lost data as a result of a bad patch
> slipping through.  Hence, patches don't go in until after significant
> review and testing.
> 
> As a result, #1, patches that are don't add value, and are large,
> simply won't get applied.  Period.  Avoiding the downside of lots of
> people losing data is ****far**** more than your OCD wanting me to use
> pr_warn(...) instead of printk(KERN_WARN, ...).

I believe it's less OCD than you do.

Using a facility to prefix dmesg output consistently
per subsystem adds value in my opinion.

> If you want your style patches to go in, break them into smaller
> chunks, or I *will* ignore them.

OK, I'll resubmit it as micropatches.

cheers, Joe

Re: [PATCH] checkpatch: Suggest pr_<level> over printk(KERN_<LEVEL>

[Ted Ts'o]

On Wed, Mar 14, 2012 at 06:05:36AM -0700, Joe Perches wrote:
> Patchwork queues are pretty useless when patches entered
> do not have their status updated for long periods.
> 
> The patch I sent in August 2011 shows "new" rather than
> have an appropriate status.
> 
> If you actually use patchwork, though it seems you don't,
> I think you should just mark every patch that's new as
> rejected and start over.

I use it, but not in the way you think I should be using it.  Your not
getting to your will on other kernel developers is what this thread is
all all about, ultimately.

I don't get to work on ext4 full time, and so every minute I put on it
has to not a be a waste of time.  This includes updating status
messages for patches that aren't obviously not applicable, or
superceded, but rather something that I might get to look at later.

	    	       		      	      - Ted

Re: [PATCH] checkpatch: Suggest pr_<level> over printk(KERN_<LEVEL>

[Joe Perches]

On Wed, 2012-03-14 at 08:34 -0400, Ted Ts'o wrote:
> Look, the reason why I care about patch noise is because I have a huge
> patch backlog.  Take a look at this:
>       http://patchwork.ozlabs.org/project/linux-ext4/list/

On Wed, 2012-03-14 at 09:45 -0400, Ted Ts'o wrote:
> On Wed, Mar 14, 2012 at 06:05:36AM -0700, Joe Perches wrote:
> > If you actually use patchwork, though it seems you don't,
> > I think you should just mark every patch that's new as
> > rejected and start over.
> I use it, but not in the way you think I should be using it.

Given the information in the link you sent,
it's not possible for anyone but you to
assess your patch backlog.

cheers, Joe