diff mbox

[v2] checkpatch: add double empty line check

Message ID 1353438635.10779.10.camel@lb-tlvb-eilong.il.broadcom.com
State Not Applicable, archived
Delegated to: David Miller
Headers show

Commit Message

Eilon Greenstein Nov. 20, 2012, 7:10 p.m. UTC 
On Tue, 2012-11-20 at 18:36 +0200, Andy Whitcroft wrote:
> On Tue, Nov 20, 2012 at 04:14:17PM +0000, Andy Whitcroft wrote:
> > On Tue, Nov 20, 2012 at 06:06:10PM +0200, Eilon Greenstein wrote:
> > > I'm only testing the nextline if the current line is newly added. If I
> > > got it right, when a line is newly added, the next line can be:
> > > a. another new line
> > > b. existing line (provided for context)
> > > c. Does not exist since this is the end of the file (I missed this one
> > > originally)
> > > 
> > > It cannot just jump to the next hunk and it cannot be a deleted line,
> > > right?
> 
> Oh and in theory at least it could be a - line, though diff never
> generates things in that order.
> 

Andy - I could not find any other perl warnings. In the current
suggestion, only $line and $prevline are being accessed unconditionally
and $rawlines[$linenr] is accessed only after checking it exists - so it
seems safe.

About the logic - true, if diff will show deleted lines after newly
added lines, some new double line segments will be missed. However, it
seems like few other things will break if diff will start acting out
like that. The suggestion you posted earlier will miss those as well,
and starting to check for this weird case (of deleted lines after the
added lines) does not seem right.

So this patch seems safe, it does not generate false positives and it
does catch all the cases we can currently generate with diff - can you
please consider applying it to checkpatch?

I'm posting it again below for easier reference, please let me know if
you would like me to send it in a clean email separately.

Thanks,
Eilon

From 403038375a9c09046631f674d14c221758a0de61 Mon Sep 17 00:00:00 2001
From: Eilon Greenstein <eilong@broadcom.com>
Date: Tue, 20 Nov 2012 21:05:30 +0200
Subject: [PATCH] checkpatch: add double empty line check

Signed-off-by: Eilon Greenstein <eilong@broadcom.com>
---
 scripts/checkpatch.pl |   13 +++++++++++++
 1 file changed, 13 insertions(+)

Comments

Andy Whitcroft Nov. 20, 2012, 7:32 p.m. UTC | #1 
On Tue, Nov 20, 2012 at 09:10:35PM +0200, Eilon Greenstein wrote:

> About the logic - true, if diff will show deleted lines after newly
> added lines, some new double line segments will be missed. However, it
> seems like few other things will break if diff will start acting out
> like that. The suggestion you posted earlier will miss those as well,
> and starting to check for this weird case (of deleted lines after the
> added lines) does not seem right.

Actually the version I sent should indeed cope with the deleted lines
regardless of order.  It was cirtainly intended to.

-apw
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Andy Whitcroft Nov. 20, 2012, 8:11 p.m. UTC | #2 
On Tue, Nov 20, 2012 at 07:32:49PM +0000, Andy Whitcroft wrote:
> On Tue, Nov 20, 2012 at 09:10:35PM +0200, Eilon Greenstein wrote:
> 
> > About the logic - true, if diff will show deleted lines after newly
> > added lines, some new double line segments will be missed. However, it
> > seems like few other things will break if diff will start acting out
> > like that. The suggestion you posted earlier will miss those as well,
> > and starting to check for this weird case (of deleted lines after the
> > added lines) does not seem right.
> 
> Actually the version I sent should indeed cope with the deleted lines
> regardless of order.  It was cirtainly intended to.

... and I think I thought of a couple more corner cases neither solution
will find.  So I am going to go away and make up a proper set of tests
for this apparently simple change.  As it is really annoying when it
false positives.  I will post against when I have something which works.

-apw
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Eilon Greenstein Nov. 20, 2012, 8:26 p.m. UTC | #3 
On Tue, 2012-11-20 at 20:11 +0000, Andy Whitcroft wrote:
> > 
> > Actually the version I sent should indeed cope with the deleted lines
> > regardless of order.  It was cirtainly intended to.
> 
> ... and I think I thought of a couple more corner cases neither solution
> will find.  So I am going to go away and make up a proper set of tests
> for this apparently simple change.  As it is really annoying when it
> false positives.  I will post against when I have something which works.
> 

Thanks Andy!

I will assist in testing it on all the scenarios I have created once you
post it.

I appreciate you taking the time to look into adding a test for this
issue.

Thanks,
Eilon


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

Patch

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 21a9f5d..c0c610c 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -3579,6 +3579,19 @@  sub process {
 			WARN("EXPORTED_WORLD_WRITABLE",
 			     "Exporting world writable files is usually an error. Consider more restrictive permissions.\n" . $herecurr);
 		}
+
+# check for double empty lines
+		if ($line =~ /^\+\s*$/) {
+			my $nextline = "";
+			if (defined($rawlines[$linenr])) {
+				$nextline = $rawlines[$linenr];
+			}
+			if ($nextline =~ /^\s*$/ ||
+			    $prevline =~ /^\+?\s*$/ && $nextline !~ /^\+\s*$/) {
+				CHK("DOUBLE_EMPTY_LINE",
+				    "One empty line should be sufficient. Consider removing this one.\n" . $herecurr);
+			}
+		}
 	}
 
 	# If we have no input at all, then there is nothing to report on