Fix line number reporting when checking source files (as opposed to

patches)
Signed-off-by: Mike D. Day <ncmike@us.ibm.com>

---

checkpatch.pl-next (as of Oct. 12) reports lines incorrectly when

checking source files as opposed to checking patches.
A line number will show up twice in a normal message, 3 times when

using the --emacs option. Here is an example using the 10/12 version:
~/slp-devel/src/cmd-utils/slp_query/slp_query.c:9:

ERROR: trailing whitespace #13: FILE:

~/slp-devel/src/cmd-utils/slp_query/slp_query.c:10:
In the report above, checkpatch reports three different line numbers:

9, 13, and 10. When using the --file option, all numbers are supposed

to be the same. One of the errors is caused by initializing the prefix

too early. By moving that line from 606 to 675 there an improvement,

though still incorrect:
~/slp-devel/src/cmd-utils/slp_query/slp_query.c:10:

ERROR: trailing whitespace #13: FILE:

~/slp-devel/src/cmd-utils/slp_query/slp_query.c:10: 
To check a sourcefie, checkpatch diffs that file with /dev/null and

pipes the resulting patch into stdin. Checkpatch uses different

variables to keep track of a patch ($linenr) and the corresponding

line in the sourcefile ($realline). Since the sourcefile *is* a patch

in this case, the line number is reported incorrectly.
To fix this, checkpatch needs to alter the variable it uses for the

source line when the --file option is used, as below. 
$prefix = "$filename:$realline: " if ($emacs && $file);

$prefix = "$filename:$linenr: " if ($emacs && !$file);
--- checkpatch.pl-next-10-12	2007-10-12 13:39:21.000000000 -0400

+++ checkpatch.pl	2007-10-12 14:30:55.000000000 -0400

@@ -606,7 +606,6 @@
 		my $rawline = $line;
-		$prefix = "$filename:$realline: " if ($emacs);
 #extract the filename as it passes

 		if ($line=~/^\+\+\+\s+(\S+)/) {

@@ -665,13 +664,16 @@

 		}
 #make up the handle for any error we report on this line

-		$here = "#$linenr: ";

+		$here = "#$linenr: " if (!$f...
[ message continues ]

Sorry you've had to fix this about 4 times, mostly because of ongoing

changes, and slow replication getting in the way.  I've applied this

and you should find it in -next when replication hits.  md5sum is

below of the version with it in, so you can make sure you've got

the right one.
54f053c50265e44a6041e3147dc66a69  checkpatch.pl
-apw

-

Andy, I've tested the --emacs feature in the above latest

checkpatch.pl-next.  Below is a patch that completes the functionality of

the --emacs option: it ensures that only the cc-style error messages are

printed, no extra context lines or caret lines, no extra newlines, etc.

Although this patch changes every call to a message-producing function, it

is a trivial change, and I believe it's the cleanest way to handle the

separation between the terse cc-style messages and the verbose default

messages.  With this patch, I can finally test a single source file as

follows:
$ ./scripts/checkpatch.pl -q -q --emacs --file path/name/to/file
(Yes, I need two -q).
Cheers,

Erez.
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl

index 13de0e2..051b354 100755

--- a/scripts/checkpatch.pl

+++ b/scripts/checkpatch.pl

@@ -511,18 +511,21 @@ sub report_dump {

 	@report;

 }

 sub ERROR {

-	report("ERROR: $_[0]\n");

+	report("ERROR: $_[0]");

+	report("$_[1]\n") if (!$emacs);

 	our $clean = 0;

 	our $cnt_error++;

 }

 sub WARN {

-	report("WARNING: $_[0]\n");

+	report("WARNING: $_[0]");

+	report("$_[1]\n") if (!$emacs);

 	our $clean = 0;

 	our $cnt_warn++;

 }

 sub CHK {

 	if ($check) {

-		report("CHECK: $_[0]\n");

+		report("CHECK: $_[0]");

+		report("$_[1]\n") if (!$emacs);

 		our $clean = 0;

 		our $cnt_chk++;

 	}

@@ -663,18 +666,18 @@ sub process {

 			# This is a signoff, if ugly, so do not double report.

 			$signoff++;

 			if (!($line =~ /^\s*Signed-off-by:/)) {

-				WARN("Signed-off-by: is the preferred form\n" .

+				WARN("Signed-off-by: is the preferred form\n",

 					$herecurr);

 			}

 			if ($line =~ /^\s*signed-off-by:\S/i) {

-				WARN("need space after Signed-off-by:\n" .

+				WARN("need space after Signed-off-by:\n",

 					$herecurr);

 			}

 		}
 # Check for wrappage within a valid hunk of the file

 		if ($realcnt != 0 && $line !~ m{^(?:\+|-| |$)}) {

-			ERROR("patch seems to be corrupt (line wrapped?)\n" .

+			ERROR("patch seems to be ...
[ message continues ]

Ok I don't understand why the rest of the lines are a problem?  At least

with emacs the extra context lines are just ignored right?  Are you

trying to use this as a summary?
-apw

-

Andy,
I'm trying to minimize excess stuff that's not necessarily useful for

everyone, and to match what is done elsewhere.  For example, I don't need

those extra newlines and find them a distraction.  And if get an error

message such as "put a space after a comma", I don't really need a caret

sign to point to the exact char in the line where it is.
When g/cc prints out errors from a compile, the errors are one per line, w/o

any additional context lines, caret markers, newlines, etc.  grep -n does

the same (also useful inside emacs).  So I'm just asking for a way to have

the same terse format.
If you feel that the extra info is still useful for some people, then can

you please provide a way for some people like me to turn off the extra lines
Thanks,

Erez.

-

Ahh, ok so this isn't quite the same as the --emacs madness.  Will see

what we can do.  I saw your patch changing every caller, and perhaps we

can avoid that.
-apw

-

latest checkpatch.pl works really well on sched.c.
there's only one problem left, this bogus false positive warning

reappeared:
  WARNING: braces {} are not necessary for single statement blocks

  #5710: FILE: sched.c:5710:

  +       if (parent->groups == parent->groups->next) {

  +               pflags &= ~(SD_LOAD_BALANCE |

  +                               SD_BALANCE_NEWIDLE |

  +                               SD_BALANCE_FORK |

  +                               SD_BALANCE_EXEC |

  +                               SD_SHARE_CPUPOWER |

  +                               SD_SHARE_PKG_RESOURCES);

  +       }
(there's another place in sched.c that trips this up too.)
i think it has been pointed out numerous times that it is perfectly fine

to use curly braces for multi-line single-statement blocks. That

includes simple cases like this too:
	if (x) {

		/* do y() */

		y();

	}
it's perfectly legitimate, in fact more robust. So if checkpatch.pl

wants to make any noise about such constructs it should warn about the

_lack_ of curly braces in every multi-line condition block _except_ the

only safe single-line statement:
	if (x)

		y();
thanks,
	Ingo

-

It actually never went away, some of the wronger reports went away such

as counting a commented statement as a single statement.  The check for

length didn't make the cut for 0.11, as I was still thinking about

whether we wanted a subjective check on statements over and above the
Yes and the comment in there actually counts as a statement for counting

statement purposes.  
The plan is to move to counting lines and only winge on exactly one

line.  I have half a mind to make a subjective check on statements and a
Indeed.  We should probabally do more on the indentation checks in

general.  The current direct check for:
	if (foo);

		bar();
Could probabally be generalised to look for this kind of error:
	if (foo)

		bar();

		baz();

	one();
-apw

-

Ok, checkpatch.pl-next should now no longer trip up on this statement.
-apw

-

indeed it works fine - thanks! kernel/sched.c is now down to:
  total: 0 errors, 1 warnings, 6990 lines checked
(and that 1 warning is a correct warning from checkpatch.pl.)
	Ingo

-

detecting that would be awesome - it's often the sign of a real bug

because the intent is often to have bar() and baz() in the conditional

block.
	Ingo

-

Should probably detect these as well (if not yet being done):
	if (abc)

#if...

		def();

		ghi();

#e...
...plus this one:
	if (abc)

#if...

		def();

#endif

	ghi();
...Both of them are clearly bugs.
...and this where either indentation has to be fixed or the bug corrected:
	if (abc)

	def();
--

 i.

-

This is more useful operating on an entire file, so the script can see

all the context.
A 'gcc -Windentation-contradicts-codeflow -Werror' would be nice.
--

Do not meddle in the internals of kernels, for they are subtle and quick to panic.
-

I had a tool long ago on the Amiga which did exactly that. It double checked

that the indentation matched the code flow and flagged any differences.

It was a dumb tool -- not a full C parser; but worked just fine. I got

it from some free software floppy disk. Unfortunately I forgot the

name and was never able to find it again.
These days emacs can do it for me though, but doing it in batch would

be nicer.
-Andi

-

there's "checkpatch --file" for complete files, so it can see the full

context if the user passes it in.
	Ingo

-

Indeed so.  checkpatch uses all the context it can when making a

decision.  Often 3 lines carries enough history to figure this out.
-apw

-

This is new, I guess?
[jgarzik@pretzel linux-2.6]$  scripts/checkpatch.pl --file

drivers/ata/libata-eh.c

Unknown option: file
Such an option would be quite useful, but does not appear in upstream.
Also, upstream lacks support for "-h" and "--help" so you get to guess

at the options.
	Jeff
-

Yep.
Andy has sent latest to akpm, so it could be pushed to Linus as well.
and you can also get latest at

http://www.kernel.org/pub/linux/kernel/people/apw/checkpatch/
---

~Randy

-