Message-ID: <1353490921.6559.40.camel@lb-tlvb-eilong.il.broadcom.com>
Date:	Wed, 21 Nov 2012 11:42:01 +0200
From:	"Eilon Greenstein" <eilong@...adcom.com>
To:	"Joe Perches" <joe@...ches.com>,
	"Andy Whitcroft" <apw@...onical.com>
cc:	"David Rientjes" <rientjes@...gle.com>,
	linux-kernel@...r.kernel.org, netdev <netdev@...r.kernel.org>
Subject: Re: [PATCH v2] checkpatch: add double empty line check

On Tue, 2012-11-20 at 15:41 -0800, Joe Perches wrote:
> On Tue, 2012-11-20 at 23:19 +0000, Andy Whitcroft wrote:
> > On Tue, Nov 20, 2012 at 01:58:48PM -0800, Joe Perches wrote:
> > 
> > > +# check for multiple blank lines, warn only on the second one in a block
> > > +		if ($rawline =~ /^.\s*$/ &&
> > > +		    $prevrawline =~ /^.\s*$/ &&
> > > +		    $linenr != $last_blank_linenr + 1) {
> > > +			CHK("DOUBLE_EMPTY_LINE",
> > > +			    "One blank line separating blocks is generally sufficient\n" . $herecurr);
> > > +			$last_blank_linenr = $linenr;
> > > +		}
> > > +
> > >  # check for line continuations in quoted strings with odd counts of "
> > >  		if ($rawline =~ /\\$/ && $rawline =~ tr/"/"/ % 2) {
> > >  			WARN("LINE_CONTINUATIONS",
> > 
> > Pretty sure that will fail with combination which have removed lines.
> 
> Not as far as I can tell.
> Deleted lines followed by inserted lines seem
> to work OK.
> 
> This check is located after the test that ensures
> the current $line/$rawline is an insertion.
> 

But you do not look at the next line, so you will miss something like
that:

diff --git a/test.c b/test.c
index e3c46d4..e1c6ffc 100644
--- a/test.c
+++ b/test.c
@@ -15,7 +15,8 @@
  * something
  * something
  * something
- * next line was already empty */
+ * next line was already empty, but I'm adding another one now*/
+
 
 /* something else
  * something else

> > I have a version here which I am testing with the combinations I have
> > isolated to far ...
> 
> Enjoy.
> Can you please test my proposal against those combinations too?
> 

The way I see it, we have to handle the following cases:
a. The patch adds more than a single consecutive empty line - easy
enough, the only "problem" here is to warn only once and there are many
ways to do that.
b. The patch is adding a new empty line after an existing empty line -
for that, we must check the previous line.
c. The patch is adding a new empty line before an existing empty line -
for that, we must check the next line. If we are already checking the
next line, we can tell if this is the last empty line added and
therefore do not need to save anything in order to warn only once per
block.

My version of the patch addresses all 3 cases above, and I do not see
how we can do it without looking at the next line and the previous line
- so I think it is a valid approach.

The only identified down side is that it might fail to warn about double
empty lines if we will find a diff utility that will add the deleted
lines after the inserted lines - but even in that case, it will not
generate any annoying false positives and no other perl warnings. To
address this issue, I can add a loop that will look forward if the next
line after a newly added empty line is a deleted line, but I think this
is excessive and I will only be able to test it on manually generated
files since the diff utilities I'm familiar with are behaving nicely and
delete before adding.

Anyway, I'm looking forward for your version.

Thanks,
Eilon


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

Powered by blists - more mailing lists