[PATCH] scripts/checkpatch.pl: added test for repeated lines

[PATCH] scripts/checkpatch.pl: added test for repeated lines

[Edwin van Vliet]

Repeated lines may indicate a bug or code that needs clarification. If the
repeated line is intentional, an extra comment may be helpful for reviewers
since the repeated pattern is likely to draw attention.

Signed-off-by: Edwin van Vliet <edwin@cheatah.nl>
---
 scripts/checkpatch.pl |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index b0aa2c6..4b50496 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -1479,6 +1479,11 @@ sub process {
 			WARN("adding a line without newline at end of file\n" . $herecurr);
 		}
 
+# check for repeated lines which may indicate bugs or lack of clarity
+		if ($rawline eq $prevrawline) {
+			WARN("repeated line\n" . $herecurr);
+		}
+
 # Blackfin: use hi/lo macros
 		if ($realfile =~ m@arch/blackfin/.*\.S$@) {
 			if ($line =~ /\.[lL][[:space:]]*=.*&[[:space:]]*0x[fF][fF][fF][fF]/) {
-- 
1.7.5.4

Re: [PATCH] scripts/checkpatch.pl: added test for repeated lines

[Joe Perches]

On Sun, 2011-07-10 at 20:18 +0200, Edwin van Vliet wrote:
> Repeated lines may indicate a bug or code that needs clarification. If the
> repeated line is intentional, an extra comment may be helpful for reviewers
> since the repeated pattern is likely to draw attention.
[]
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
[]
> @@ -1479,6 +1479,11 @@ sub process {
> +# check for repeated lines which may indicate bugs or lack of clarity
> +		if ($rawline eq $prevrawline) {
> +			WARN("repeated line\n" . $herecurr);
> +		}
> +

Interest concept, but I think it needs to check for
comment lines and blank lines and such.

Also, there are uses of appropriate multiple close braces
on consecutive lines like:

	switch (foo) {
	case bar: {
		etc...
	}
	}

Re: [PATCH] scripts/checkpatch.pl: added test for repeated lines

[Edwin van Vliet]

On 10-7-2011 20:49, Joe Perches wrote:
> On Sun, 2011-07-10 at 20:18 +0200, Edwin van Vliet wrote:
>> Repeated lines may indicate a bug or code that needs clarification. If the
>> repeated line is intentional, an extra comment may be helpful for reviewers
>> since the repeated pattern is likely to draw attention.
> []
>> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> []
>> @@ -1479,6 +1479,11 @@ sub process {
>> +# check for repeated lines which may indicate bugs or lack of clarity
>> +		if ($rawline eq $prevrawline) {
>> +			WARN("repeated line\n" . $herecurr);
>> +		}
>> +
> 
> Interest concept, but I think it needs to check for
> comment lines and blank lines and such.
> 
> Also, there are uses of appropriate multiple close braces
> on consecutive lines like:
> 
> 	switch (foo) {
> 	case bar: {
> 		etc...
> 	}
> 	}

You might be correct about empty lines and comment lines, but there are
actually very few reasons to have a repeated line. Your example is a
good one, I actually ran a script on the entire kernel source, there are
now exactly 9271 occurances of repeated lines in all .c files, and a
double closing bracket on the same position occurs 147 times, which is
not very often.

Repeated lines draw (my) attention, and are likely to confuse. Even
empty lines and empty comment lines take up space, which means less code
is displayed on a screen.

Do you reckon this test would lead to programmers trying to "fool" the
test and actually insert extra tabs to work around it? It's only a
warning, just like there are for lines over 80 characters long.

If the final goal of the checkpatch.pl script is for the entire kernel
source code to not generate any warnings at all, then why are they only
warnings and not errors?

I understand your concerns and agree this might need a little testing
but still this may be helpful to spot comment junk like in
drivers/usb/serial/io_edgeport.c or the obvious double define error in
drivers/infiniband/hw/qib/qib_iba7322.c.
Or the undesired triple assignment in drivers/staging/bcm/nvm.c, since
psFlash2xBitMap->Reserved0 = 0; being called only once is quite enough,
and actually I think it's a possible bug which can happen if a
programmer copies a line a few times and thereafter alter them a little
bit, which in this case was probably forgotten.

Please have a look at fs/sync.c and look for "sync_filesystems(0)". It's
repeated twice, but right above two lines of comment clarify why this is
done. That's exactly what I think is necessary.
Now have a look at drivers/media/dvb/frontends/nxt200x.c and look for
"nxt200x_microcontroller_stop(state)" being repeated. I think this code
needs exactly the same kind of comment. Was the function being called
twice intentional? If so, then why? Or is it a bug?

Sure, double #endif's and closing brackets will cause a warning, quite a
number of false positives are inevitable. But it will hopefully help to
prevent or spot errors too, and it might lead to better documentation of
certain routines as well.

On the other hand, double closing brackets caused by wrong indentation
would have been spotted by another test, so you might be right to make a
few exceptions (closing bracket, #endif?).

Re: [PATCH] scripts/checkpatch.pl: added test for repeated lines

[Joe Perches]

On Sun, 2011-07-10 at 22:23 +0200, Edwin van Vliet wrote:
> On 10-7-2011 20:49, Joe Perches wrote:
> > On Sun, 2011-07-10 at 20:18 +0200, Edwin van Vliet wrote:
> >> Repeated lines may indicate a bug or code that needs clarification. If the
> >> repeated line is intentional, an extra comment may be helpful for reviewers
> >> since the repeated pattern is likely to draw attention.
> > []
> >> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> > []
> >> @@ -1479,6 +1479,11 @@ sub process {
> >> +# check for repeated lines which may indicate bugs or lack of clarity
> >> +		if ($rawline eq $prevrawline) {
> >> +			WARN("repeated line\n" . $herecurr);
> >> +		}
> >> +
> > Interest concept, but I think it needs to check for
> > comment lines and blank lines and such.
> > Also, there are uses of appropriate multiple close braces
> > on consecutive lines like:
> > 	switch (foo) {
> > 	case bar: {
> > 		etc...
> > 	}
> > 	}
[]
> You might be correct about empty lines and comment lines, but there are
> actually very few reasons to have a repeated line.
[]
> Repeated lines draw (my) attention, and are likely to confuse.

I don't disagree, I just think your patch needs to handle
some obvious exceptions.

> Do you reckon this test would lead to programmers trying to "fool" the
> test and actually insert extra tabs to work around it?

No.

> If the final goal of the checkpatch.pl script is for the entire kernel
> source code to not generate any warnings at all,

It's not.

> I understand your concerns and agree this might need a little testing

and expanding and improving.

[]
> Sure, double #endif's and closing brackets will cause a warning, quite a
> number of false positives are inevitable.

Not if your patch handles them appropriately.
Maybe use both CHK and WARN.

> But it will hopefully help to
> prevent or spot errors too, and it might lead to better documentation of
> certain routines as well.

True.  But don't expect me to ack or add a sign-off to
a patch that has obvious defects or doesn't do the
thing it purports to do well.

cheers, Joe