Patchwork [v2] checkpatch: add double empty line check

login
register
mail settings
Submitter Eilon Greenstein
Date Nov. 21, 2012, 9:42 a.m.
Message ID <1353490921.6559.40.camel@lb-tlvb-eilong.il.broadcom.com>
Download mbox | patch
Permalink /patch/200616/
State Not Applicable
Delegated to: David Miller
Headers show

Comments

Eilon Greenstein - Nov. 21, 2012, 9:42 a.m.
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:


> > 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@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Joe Perches - Nov. 21, 2012, 3:01 p.m.
On Wed, 2012-11-21 at 11:42 +0200, Eilon Greenstein wrote:
> 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*/
> +

Hi Eilon.

Thanks for the test case.

That's true, but I'm OK with missing a few cases in the
search for simplicity as long as there aren't significant
false positives.

For instance the next test
# check for line continuations in quoted strings with odd counts of "
		if ($rawline =~ /\\$/ && $rawline =~ tr/"/"/ % 2) {
			WARN("LINE_CONTINUATIONS",

That fails if the rawline is:
	"\"" \
Does it matter much?  Probably not.
I suppose that test could be improved by using $line.

checkpatch isn't a perfect tool.  Given how it's constructed,
I doubt it ever could be.

No doubt you and Andy will find a better solution.


--
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. 21, 2012, 3:45 p.m.
On Wed, 2012-11-21 at 07:01 -0800, Joe Perches wrote:
> checkpatch isn't a perfect tool.  Given how it's constructed,
> I doubt it ever could be.

Joe - I completely agree, this is why I'm not to concern about the
potential miss in the version I suggested.

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

Patch

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