Changes from previous attempt:
- Use CHK instead of WARN
- Issue only one warning per empty lines block
Signed-off-by: Eilon Greenstein <[email protected]>
---
scripts/checkpatch.pl | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 21a9f5d..13d264f 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -3579,6 +3579,14 @@ 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*$/ &&
+ ($rawlines[$linenr] =~ /^\s*$/ ||
+ $prevline =~ /^\+?\s*$/ && $rawlines[$linenr] !~ /^\+\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
--
1.7.9.5
On Sat, Nov 17, 2012 at 01:17:37PM +0200, Eilon Greenstein wrote:
> Changes from previous attempt:
> - Use CHK instead of WARN
> - Issue only one warning per empty lines block
>
> Signed-off-by: Eilon Greenstein <[email protected]>
> ---
> scripts/checkpatch.pl | 8 ++++++++
> 1 file changed, 8 insertions(+)
>
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index 21a9f5d..13d264f 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -3579,6 +3579,14 @@ 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*$/ &&
> + ($rawlines[$linenr] =~ /^\s*$/ ||
> + $prevline =~ /^\+?\s*$/ && $rawlines[$linenr] !~ /^\+\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
In your previous version you indicated you would be emiting one per group
of lines, I do not see how this does that. Also this fails if the fragment
is at the top of the hunk emiting a perl warning. We should probabally
use the suppress approach.
How about something like the below.
-apw
>From 848ebffa8656a1ff96a91788ec0f1c04dab9c3e9 Mon Sep 17 00:00:00 2001
From: Andy Whitcroft <[email protected]>
Date: Sat, 17 Nov 2012 13:17:37 +0200
Subject: [PATCH] checkpatch: strict warning for multiple blank lines
Signed-off-by: Andy Whitcroft <[email protected]>
---
scripts/checkpatch.pl | 11 +++++++++++
1 file changed, 11 insertions(+)
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index f18750e..dbc68f3 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -1411,6 +1411,7 @@ sub process {
my %suppress_whiletrailers;
my %suppress_export;
my $suppress_statement = 0;
+ my $suppress_multipleblank = -1;
# Pre-scan the patch sanitizing the lines.
# Pre-scan the patch looking for any __setup documentation.
@@ -1521,6 +1522,7 @@ sub process {
%suppress_whiletrailers = ();
%suppress_export = ();
$suppress_statement = 0;
+ $suppress_multipleblank = -1;
next;
# track the line number as we move through the hunk, note that
@@ -1930,6 +1932,15 @@ sub process {
"use the SSYNC() macro in asm/blackfin.h\n" . $herevet);
}
+# multiple blank lines.
+ if ($line =~ /^-/ || ($suppress_multipleblank == $linenr && $line =~ /^[ \+]\s*$/)) {
+ $suppress_multipleblank++;
+ } elsif ($prevline =~ /^\+\s*$/ and $line =~ /^\+\s*$/) {
+ $suppress_multipleblank = $linenr + 1;
+ CHK("MULTIPLE_EMPTY_LINE",
+ "One empty line should be sufficient. Consider removing this one.\n" . $herecurr);
+ }
+
# Check for potential 'bare' types
my ($stat, $cond, $line_nr_next, $remain_next, $off_next,
$realline_next);
--
1.7.10.4
On Tue, 2012-11-20 at 11:52 +0000, Andy Whitcroft wrote:
Andy, thanks for reviewing this patch.
> On Sat, Nov 17, 2012 at 01:17:37PM +0200, Eilon Greenstein wrote:
> > Changes from previous attempt:
> > - Use CHK instead of WARN
> > - Issue only one warning per empty lines block
> >
> > Signed-off-by: Eilon Greenstein <[email protected]>
> > ---
> > scripts/checkpatch.pl | 8 ++++++++
> > 1 file changed, 8 insertions(+)
> >
> > diff --git a/scripts/checkpatch.pl b/scripts/linescheckpatch.pl
> > index 21a9f5d..13d264f 100755
> > --- a/scripts/checkpatch.pl
> > +++ b/scripts/checkpatch.pl
> > @@ -3579,6 +3579,14 @@ 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*$/ &&
> > + ($rawlines[$linenr] =~ /^\s*$/ ||
> > + $prevline =~ /^\+?\s*$/ && $rawlines[$linenr] !~ /^\+\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
>
> In your previous version you indicated you would be emiting one per group
> of lines, I do not see how this does that.
This is what I'm testing:
Only if the current line is a new blank line and:
if the next line is empty but not newly added (this is the part that
will make sure we get only one warning for a bunch of new lines - only
the last newly added line will hit this condition)
or
if the previous line was empty (either new empty line or existing empty
line) and the next line is not a new empty line (so we will issue just
one warning).
I tested it on few examples, and did not see a problem. Can you share an
example where it issues more than a single warning for a newly
introduced consecutive new lines?
> Also this fails if the fragment
> is at the top of the hunk emiting a perl warning.
I did not see this warning. Can you please share this example? I tried
adding a couple of empty lines at the beginning of a file and it seemed
to work just fine for me (using perl v5.14.2).lines
> We should probabally
> use the suppress approach.
>
> How about something like the below.
>
> -apw
>
>
> From 848ebffa8656a1ff96a91788ec0f1c04dab9c3e9 Mon Sep 17 00:00:00 2001
> From: Andy Whitcroft <[email protected]>
> Date: Sat, 17 Nov 2012 13:17:37 +0200
> Subject: [PATCH] checkpatch: strict warning for multiple blank lines
>
> Signed-off-by: Andy Whitcroft <[email protected]>
> ---
> scripts/checkpatch.pl | 11 +++++++++++
> 1 file changed, 11 insertions(+)
>
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index f18750e..dbc68f3 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -1411,6 +1411,7 @@ sub process {
> my %suppress_whiletrailers;
> my %suppress_export;
> my $suppress_statement = 0;
> + my $suppress_multipleblank = -1;
>
> # Pre-scan the patch sanitizing the lines.
> # Pre-scan the patch looking for any __setup documentation.
> @@ -1521,6 +1522,7 @@ sub process {
> %suppress_whiletrailers = ();
> %suppress_export = ();
> $suppress_statement = 0;
> + $suppress_multipleblank = -1;
> next;
>
> # track the line number as we move through the hunk, note that
> @@ -1930,6 +1932,15 @@ sub process {
> "use the SSYNC() macro in asm/blackfin.h\n" . $herevet);
> }
>
> +# multiple blank lines.
> + if ($line =~ /^-/ || ($suppress_multipleblank == $linenr && $line =~ /^[ \+]\s*$/)) {
> + $suppress_multipleblank++;
> + } elsif ($prevline =~ /^\+\s*$/ and $line =~ /^\+\s*$/) {
> + $suppress_multipleblank = $linenr + 1;
> + CHK("MULTIPLE_EMPTY_LINE",
> + "One empty line should be sufficient. Consider removing this one.\n" . $herecurr);
> + }
> +
> # Check for potential 'bare' types
> my ($stat, $cond, $line_nr_next, $remain_next, $off_next,
> $realline_next);
The problem with this version is that it only catches newly added empty
lines. But if there was an empty line before or after this newly added
empty line, there will be no warning. I would like to catch things like
that as well.
Thanks,
Eilon
On Tue, Nov 20, 2012 at 04:27:04PM +0200, Eilon Greenstein wrote:
> On Tue, 2012-11-20 at 11:52 +0000, Andy Whitcroft wrote:
>
> Andy, thanks for reviewing this patch.
>
> > On Sat, Nov 17, 2012 at 01:17:37PM +0200, Eilon Greenstein wrote:
> > > Changes from previous attempt:
> > > - Use CHK instead of WARN
> > > - Issue only one warning per empty lines block
> > >
> > > Signed-off-by: Eilon Greenstein <[email protected]>
> > > ---
> > > scripts/checkpatch.pl | 8 ++++++++
> > > 1 file changed, 8 insertions(+)
> > >
> > > diff --git a/scripts/checkpatch.pl b/scripts/linescheckpatch.pl
> > > index 21a9f5d..13d264f 100755
> > > --- a/scripts/checkpatch.pl
> > > +++ b/scripts/checkpatch.pl
> > > @@ -3579,6 +3579,14 @@ 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*$/ &&
> > > + ($rawlines[$linenr] =~ /^\s*$/ ||
> > > + $prevline =~ /^\+?\s*$/ && $rawlines[$linenr] !~ /^\+\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
> >
> > In your previous version you indicated you would be emiting one per group
> > of lines, I do not see how this does that.
>
> This is what I'm testing:
> Only if the current line is a new blank line and:
> if the next line is empty but not newly added (this is the part that
> will make sure we get only one warning for a bunch of new lines - only
> the last newly added line will hit this condition)
> or
> if the previous line was empty (either new empty line or existing empty
> line) and the next line is not a new empty line (so we will issue just
> one warning).
>
> I tested it on few examples, and did not see a problem. Can you share an
> example where it issues more than a single warning for a newly
> introduced consecutive new lines?
No indeed. That was testing failure on my behalf.
>
> > Also this fails if the fragment
> > is at the top of the hunk emiting a perl warning.
>
> I did not see this warning. Can you please share this example? I tried
> adding a couple of empty lines at the beginning of a file and it seemed
> to work just fine for me (using perl v5.14.2).lines
Ok, this is actually if it is at the bottom, not the top. So if you
have a range of lines newly added to the bottom of the file. Leading to
this warning:
Use of uninitialized value within @rawlines in pattern match (m//) at
../checkpatch/scripts/checkpatch.pl line 3586.
-apw
On Tue, 2012-11-20 at 14:43 +0000, Andy Whitcroft wrote:
> > > Also this fails if the fragment
> > > is at the top of the hunk emiting a perl warning.
> >
> > I did not see this warning. Can you please share this example? I tried
> > adding a couple of empty lines at the beginning of a file and it seemed
> > to work just fine for me (using perl v5.14.2).lines
>
> Ok, this is actually if it is at the bottom, not the top. So if you
> have a range of lines newly added to the bottom of the file. Leading to
> this warning:
>
> Use of uninitialized value within @rawlines in pattern match (m//) at
> ../checkpatch/scripts/checkpatch.pl line 3586.
Oh... of course, I should have seen that. I did not check changes at the
end of the file.
What do you say about something like that (using nextline out of
rawlines only if it defined):
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.
}
+
+# 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 you think it's appropriate, I will send a clean copy.
Thanks,
Eilon
On Tue, Nov 20, 2012 at 05:07:07PM +0200, Eilon Greenstein wrote:
> On Tue, 2012-11-20 at 14:43 +0000, Andy Whitcroft wrote:
>
> > > > Also this fails if the fragment
> > > > is at the top of the hunk emiting a perl warning.
> > >
> > > I did not see this warning. Can you please share this example? I tried
> > > adding a couple of empty lines at the beginning of a file and it seemed
> > > to work just fine for me (using perl v5.14.2).lines
> >
> > Ok, this is actually if it is at the bottom, not the top. So if you
> > have a range of lines newly added to the bottom of the file. Leading to
> > this warning:
> >
> > Use of uninitialized value within @rawlines in pattern match (m//) at
> > ../checkpatch/scripts/checkpatch.pl line 3586.
>
> Oh... of course, I should have seen that. I did not check changes at the
> end of the file.
>
> What do you say about something like that (using nextline out of
> rawlines only if it defined):
> 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.
> }
> +
> +# 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);
> + }
> + }
> }
>
>
>
You cannot really rely on nextline even if valid as it may not even be
from this hunk. This is why in my attempt I detected the top of a long
run of blanks and let the hunk start initialisation reset the detector.
-apw
>From 6556d7bc2f9447e1d179c1dd32a618b124c26e46 Mon Sep 17 00:00:00 2001
From: Andy Whitcroft <[email protected]>
Date: Sat, 17 Nov 2012 13:17:37 +0200
Subject: [PATCH] checkpatch: strict warning for multiple blank lines
Signed-off-by: Andy Whitcroft <[email protected]>
---
scripts/checkpatch.pl | 11 +++++++++++
1 file changed, 11 insertions(+)
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index fb67d47..ae01b90 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -1413,6 +1413,7 @@ sub process {
my %suppress_whiletrailers;
my %suppress_export;
my $suppress_statement = 0;
+ my $suppress_multipleblank = -1;
# Pre-scan the patch sanitizing the lines.
# Pre-scan the patch looking for any __setup documentation.
@@ -1523,6 +1524,7 @@ sub process {
%suppress_whiletrailers = ();
%suppress_export = ();
$suppress_statement = 0;
+ $suppress_multipleblank = -1;
next;
# track the line number as we move through the hunk, note that
@@ -1945,6 +1947,15 @@ sub process {
"use the SSYNC() macro in asm/blackfin.h\n" . $herevet);
}
+# multiple blank lines.
+ if ($line =~ /^-/ || ($suppress_multipleblank == $linenr && $line =~ /^[ \+]\s*$/)) {
+ $suppress_multipleblank++;
+ } elsif ($prevline =~ /^\+\s*$/ and $line =~ /^\+\s*$/) {
+ $suppress_multipleblank = $linenr + 1;
+ CHK("MULTIPLE_EMPTY_LINE",
+ "One empty line should be sufficient. Consider removing this one.\n" . $herecurr);
+ }
+
# Check for potential 'bare' types
my ($stat, $cond, $line_nr_next, $remain_next, $off_next,
$realline_next);
--
1.7.10.4
On Tue, 2012-11-20 at 15:44 +0000, Andy Whitcroft wrote:
> On Tue, Nov 20, 2012 at 05:07:07PM +0200, Eilon Greenstein wrote:
> > On Tue, 2012-11-20 at 14:43 +0000, Andy Whitcroft wrote:
> >
> > > > > Also this fails if the fragment
> > > > > is at the top of the hunk emiting a perl warning.
> > > >
> > > > I did not see this warning. Can you please share this example? I tried
> > > > adding a couple of empty lines at the beginning of a file and it seemed
> > > > to work just fine for me (using perl v5.14.2).lines
> > >
> > > Ok, this is actually if it is at the bottom, not the top. So if you
> > > have a range of lines newly added to the bottom of the file. Leading to
> > > this warning:
> > >
> > > Use of uninitialized value within @rawlines in pattern match (m//) at
> > > ../checkpatch/scripts/checkpatch.pl line 3586.
> >
> > Oh... of course, I should have seen that. I did not check changes at the
> > end of the file.
> >
> > What do you say about something like that (using nextline out of
> > rawlines only if it defined):
> > 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.
> > }
> > +
> > +# 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);
> > + }
> > + }
> > }
> >
> >
> >
>
> You cannot really rely on nextline even if valid as it may not even be
> from this hunk. This is why in my attempt I detected the top of a long
> run of blanks and let the hunk start initialisation reset the detector.
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?
>
> From 6556d7bc2f9447e1d179c1dd32a618b124c26e46 Mon Sep 17 00:00:00 2001
> From: Andy Whitcroft <[email protected]>
> Date: Sat, 17 Nov 2012 13:17:37 +0200
> Subject: [PATCH] checkpatch: strict warning for multiple blank lines
>
> Signed-off-by: Andy Whitcroft <[email protected]>
> ---
> scripts/checkpatch.pl | 11 +++++++++++
> 1 file changed, 11 insertions(+)
>
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index fb67d47..ae01b90 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -1413,6 +1413,7 @@ sub process {
> my %suppress_whiletrailers;
> my %suppress_export;
> my $suppress_statement = 0;
> + my $suppress_multipleblank = -1;
>
> # Pre-scan the patch sanitizing the lines.
> # Pre-scan the patch looking for any __setup documentation.
> @@ -1523,6 +1524,7 @@ sub process {
> %suppress_whiletrailers = ();
> %suppress_export = ();
> $suppress_statement = 0;
> + $suppress_multipleblank = -1;
> next;
>
> # track the line number as we move through the hunk, note that
> @@ -1945,6 +1947,15 @@ sub process {
> "use the SSYNC() macro in asm/blackfin.h\n" . $herevet);
> }
>
> +# multiple blank lines.
> + if ($line =~ /^-/ || ($suppress_multipleblank == $linenr && $line =~ /^[ \+]\s*$/)) {
> + $suppress_multipleblank++;
> + } elsif ($prevline =~ /^\+\s*$/ and $line =~ /^\+\s*$/) {
> + $suppress_multipleblank = $linenr + 1;
> + CHK("MULTIPLE_EMPTY_LINE",
> + "One empty line should be sufficient. Consider removing this one.\n" . $herecurr);
> + }
> +
> # Check for potential 'bare' types
> my ($stat, $cond, $line_nr_next, $remain_next, $off_next,
> $realline_next);
Please consider this patch as an example to three newly added double
empty lines that are not caught by this version but are seen in the
attempt I sent before:
diff --git a/test.c b/test.c
index e3f1362..5ced040 100644
--- a/test.c
+++ b/test.c
@@ -1,4 +1,5 @@
+
/* there is an empty line just above me (the first line in this file)
* nothing
* nothing
@@ -12,17 +13,8 @@
*/
/* There is an empty line just below me */
-/* nothing
- * nothing
- * nothing
- * nothing
- * nothing
- * nothing
- * nothing
- * nothing
- * nothing
- * nothing
- */
+
+/* just added a new empty line after deleting a segment */
/* nothing
* nothing
* nothing
@@ -36,3 +28,4 @@
*/
/* The last line (right below me) is empty */
+
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?
Mostly that would be true. If the hunk is the last hunk and adds lines
at the bottom of a file _and_ the context around it has blank lines then
something. I think that would trip up this algorithm, reporting beyond
the end of the hunk perhaps.
-apw
On Tue, 2012-11-20 at 16:14 +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?
>
> Mostly that would be true. If the hunk is the last hunk and adds lines
> at the bottom of a file _and_ the context around it has blank lines then
> something. I think that would trip up this algorithm, reporting beyond
> the end of the hunk perhaps.
I do not want to cause any perl warning, but adding a new segment that
ends with a new empty line above an existing empty line is something
that I want to catch - so checking the next line (even if it is not new)
is desired. Do you have other suggestions on how to implement something
like that?
I'm not saying that my patch is safe - I already missed a corner case
when adding a line at the end of the file, but I'm willing to run more
tests and see if I hit some perl warning. So how about running it on the
last X changes in the kernel git tree? How many tests are enough to get
reasonable confidant level?
Thanks,
Eilon
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.
-apw
On Tue, Nov 20, 2012 at 06:22:24PM +0200, Eilon Greenstein wrote:
> On Tue, 2012-11-20 at 16:14 +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?
> >
> > Mostly that would be true. If the hunk is the last hunk and adds lines
> > at the bottom of a file _and_ the context around it has blank lines then
> > something. I think that would trip up this algorithm, reporting beyond
> > the end of the hunk perhaps.
>
> I do not want to cause any perl warning, but adding a new segment that
> ends with a new empty line above an existing empty line is something
> that I want to catch - so checking the next line (even if it is not new)
> is desired. Do you have other suggestions on how to implement something
> like that?
>
> I'm not saying that my patch is safe - I already missed a corner case
> when adding a line at the end of the file, but I'm willing to run more
> tests and see if I hit some perl warning. So how about running it on the
> last X changes in the kernel git tree? How many tests are enough to get
> reasonable confidant level?
I have been testing the patches there with some fake files to try and
catch these indeed. I did incldue my take on how to solve this in
previous replies.
-apw
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 <[email protected]>
Date: Tue, 20 Nov 2012 21:05:30 +0200
Subject: [PATCH] checkpatch: add double empty line check
Signed-off-by: Eilon Greenstein <[email protected]>
---
scripts/checkpatch.pl | 13 +++++++++++++
1 file changed, 13 insertions(+)
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
--
1.7.9.5
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
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
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
On Tue, 2012-11-20 at 11:52 +0000, Andy Whitcroft wrote:
> On Sat, Nov 17, 2012 at 01:17:37PM +0200, Eilon Greenstein wrote:
> > Changes from previous attempt:
> > - Use CHK instead of WARN
> > - Issue only one warning per empty lines block
> >
> > Signed-off-by: Eilon Greenstein <[email protected]>
> > ---
> > scripts/checkpatch.pl | 8 ++++++++
> > 1 file changed, 8 insertions(+)
> >
> > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> > index 21a9f5d..13d264f 100755
> > --- a/scripts/checkpatch.pl
> > +++ b/scripts/checkpatch.pl
> > @@ -3579,6 +3579,14 @@ 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*$/ &&
> > + ($rawlines[$linenr] =~ /^\s*$/ ||
> > + $prevline =~ /^\+?\s*$/ && $rawlines[$linenr] !~ /^\+\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
>
> In your previous version you indicated you would be emiting one per group
> of lines, I do not see how this does that. Also this fails if the fragment
> is at the top of the hunk emiting a perl warning. We should probabally
> use the suppress approach.
>
> How about something like the below.
>
> -apw
>
>
> >From 848ebffa8656a1ff96a91788ec0f1c04dab9c3e9 Mon Sep 17 00:00:00 2001
> From: Andy Whitcroft <[email protected]>
> Date: Sat, 17 Nov 2012 13:17:37 +0200
> Subject: [PATCH] checkpatch: strict warning for multiple blank lines
>
> Signed-off-by: Andy Whitcroft <[email protected]>
> ---
> scripts/checkpatch.pl | 11 +++++++++++
> 1 file changed, 11 insertions(+)
>
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index f18750e..dbc68f3 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -1411,6 +1411,7 @@ sub process {
> my %suppress_whiletrailers;
> my %suppress_export;
> my $suppress_statement = 0;
> + my $suppress_multipleblank = -1;
>
> # Pre-scan the patch sanitizing the lines.
> # Pre-scan the patch looking for any __setup documentation.
> @@ -1521,6 +1522,7 @@ sub process {
> %suppress_whiletrailers = ();
> %suppress_export = ();
> $suppress_statement = 0;
> + $suppress_multipleblank = -1;
> next;
>
> # track the line number as we move through the hunk, note that
> @@ -1930,6 +1932,15 @@ sub process {
> "use the SSYNC() macro in asm/blackfin.h\n" . $herevet);
> }
>
> +# multiple blank lines.
> + if ($line =~ /^-/ || ($suppress_multipleblank == $linenr && $line =~ /^[ \+]\s*$/)) {
> + $suppress_multipleblank++;
> + } elsif ($prevline =~ /^\+\s*$/ and $line =~ /^\+\s*$/) {
> + $suppress_multipleblank = $linenr + 1;
> + CHK("MULTIPLE_EMPTY_LINE",
> + "One empty line should be sufficient. Consider removing this one.\n" . $herecurr);
> + }
> +
> # Check for potential 'bare' types
> my ($stat, $cond, $line_nr_next, $remain_next, $off_next,
> $realline_next);
What about:
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index d2d5ba1..ed4ec9d 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -1382,6 +1382,7 @@ sub process {
my $comment_edge = 0;
my $first_line = 0;
my $p1_prefix = '';
+ my $last_blank_linenr = 0;
my $prev_values = 'E';
@@ -3323,6 +3324,15 @@ sub process {
"sizeof $1 should be sizeof($1)\n" . $herecurr);
}
+# 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",
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. I
have a version here which I am testing with the combinations I have
isolated to far ...
-apw
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.
> 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?
cheers, Joe
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
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.
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