From: Constantine Shulyupin <[email protected]>
debugfs_remove() and debugfs_remove_recursive() can take a NULL, so let's check and warn about that.
Changes since v3, as Joe Perches suggested:
- removed redundant check
Changes since v2, as Joe Perches suggested:
- match whitespace around argument
Changes since v1, as Joe Perches suggested:
- added debugfs_remove_recursive
- all tests for patterns are "if (a) xxx(a)" are consolidated
Signed-off-by: Constantine Shulyupin <[email protected]>
---
scripts/checkpatch.pl | 18 +++++++++++-------
1 file changed, 11 insertions(+), 7 deletions(-)
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index f18750e..76ad9f2 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -3213,21 +3213,25 @@ sub process {
$herecurr);
}
+# check for needless "if (<foo>) fn(<foo>)" uses
+ if ($prevline =~ /\bif\s*\(\s*($Lval)\s*\)/) {
+ my $expr = '\s*\(\s*' . quotemeta($1) . '\s*\)\s*;';
+
# check for needless kfree() checks
- if ($prevline =~ /\bif\s*\(([^\)]*)\)/) {
- my $expr = $1;
- if ($line =~ /\bkfree\(\Q$expr\E\);/) {
+ if ($line =~ /\bkfree$expr/) {
WARN("NEEDLESS_KFREE",
"kfree(NULL) is safe this check is probably not required\n" . $hereprev);
}
- }
# check for needless usb_free_urb() checks
- if ($prevline =~ /\bif\s*\(([^\)]*)\)/) {
- my $expr = $1;
- if ($line =~ /\busb_free_urb\(\Q$expr\E\);/) {
+ if ($line =~ /\busb_free_urb$expr/) {
WARN("NEEDLESS_USB_FREE_URB",
"usb_free_urb(NULL) is safe this check is probably not required\n" . $hereprev);
}
+# check for needless debugfs_remove() and debugfs_remove_recursive*() checks
+ if ($line =~ /\b(debugfs_remove(?:_recursive)?)$expr/) {
+ WARN("NEEDLESS_DEBUGFS_REMOVE",
+ "$1(NULL) is safe this check is probably not required\n" . $hereprev);
+ }
}
# prefer usleep_range over udelay
--
1.7.9.5
On Sun, Nov 18, 2012 at 12:20:18AM +0200, Constantine Shulyupin wrote:
> From: Constantine Shulyupin <[email protected]>
>
> debugfs_remove() and debugfs_remove_recursive() can take a NULL, so let's check and warn about that.
>
> Changes since v3, as Joe Perches suggested:
> - removed redundant check
>
> Changes since v2, as Joe Perches suggested:
> - match whitespace around argument
>
> Changes since v1, as Joe Perches suggested:
> - added debugfs_remove_recursive
> - all tests for patterns are "if (a) xxx(a)" are consolidated
>
> Signed-off-by: Constantine Shulyupin <[email protected]>
> ---
> scripts/checkpatch.pl | 18 +++++++++++-------
> 1 file changed, 11 insertions(+), 7 deletions(-)
>
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index f18750e..76ad9f2 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -3213,21 +3213,25 @@ sub process {
> $herecurr);
> }
>
> +# check for needless "if (<foo>) fn(<foo>)" uses
> + if ($prevline =~ /\bif\s*\(\s*($Lval)\s*\)/) {
> + my $expr = '\s*\(\s*' . quotemeta($1) . '\s*\)\s*;';
> +
> # check for needless kfree() checks
> - if ($prevline =~ /\bif\s*\(([^\)]*)\)/) {
> - my $expr = $1;
> - if ($line =~ /\bkfree\(\Q$expr\E\);/) {
> + if ($line =~ /\bkfree$expr/) {
> WARN("NEEDLESS_KFREE",
> "kfree(NULL) is safe this check is probably not required\n" . $hereprev);
> }
> - }
> # check for needless usb_free_urb() checks
> - if ($prevline =~ /\bif\s*\(([^\)]*)\)/) {
> - my $expr = $1;
> - if ($line =~ /\busb_free_urb\(\Q$expr\E\);/) {
> + if ($line =~ /\busb_free_urb$expr/) {
> WARN("NEEDLESS_USB_FREE_URB",
> "usb_free_urb(NULL) is safe this check is probably not required\n" . $hereprev);
> }
> +# check for needless debugfs_remove() and debugfs_remove_recursive*() checks
> + if ($line =~ /\b(debugfs_remove(?:_recursive)?)$expr/) {
> + WARN("NEEDLESS_DEBUGFS_REMOVE",
> + "$1(NULL) is safe this check is probably not required\n" . $hereprev);
> + }
> }
>
> # prefer usleep_range over udelay
This all looks sensible, though we still have three blocks doing the
same thing. How about we standardise this check into a single check,
generating the capacity from the matched name.
Something like the below on top of V4.
-apw
>From 676ce396a349f2242f80e9b2c5fb68584ec09f14 Mon Sep 17 00:00:00 2001
From: Andy Whitcroft <[email protected]>
Date: Tue, 20 Nov 2012 14:27:59 +0000
Subject: [PATCH] checkpatch: consolidate if (foo) bar(NULL) checks
Signed-off-by: Andy Whitcroft <[email protected]>
---
scripts/checkpatch.pl | 19 ++++++++-----------
1 file changed, 8 insertions(+), 11 deletions(-)
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 313617b..6660246 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -3229,19 +3229,16 @@ sub process {
my $expr = '\s*\(\s*' . quotemeta($1) . '\s*\)\s*;';
# check for needless kfree() checks
- if ($line =~ /\bkfree$expr/) {
- WARN("NEEDLESS_KFREE",
- "kfree(NULL) is safe this check is probably not required\n" . $hereprev);
- }
# check for needless usb_free_urb() checks
- if ($line =~ /\busb_free_urb$expr/) {
- WARN("NEEDLESS_USB_FREE_URB",
- "usb_free_urb(NULL) is safe this check is probably not required\n" . $hereprev);
- }
# check for needless debugfs_remove() and debugfs_remove_recursive*() checks
- if ($line =~ /\b(debugfs_remove(?:_recursive)?)$expr/) {
- WARN("NEEDLESS_DEBUGFS_REMOVE",
- "$1(NULL) is safe this check is probably not required\n" . $hereprev);
+ if ($line =~ /\b(kfree|usb_free_urb|debugfs_remove(?:_recursive)?)$expr/) {
+ my $func = $1;
+ my $func_capacity = "NEEDLESS_$1";
+
+ $func_capacity =~ s/(.$)/\U$1\E/;
+
+ WARN($func_capacity,
+ "$func(NULL) is safe this check is probably not required\n" . $hereprev);
}
}
--
1.7.10.4
On Tue, 2012-11-20 at 14:29 +0000, Andy Whitcroft wrote:
> This all looks sensible, though we still have three blocks doing the
> same thing. How about we standardise this check into a single check,
> generating the capacity from the matched name.
[]
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
[]
> + if ($line =~ /\b(kfree|usb_free_urb|debugfs_remove(?:_recursive)?)$expr/) {
> + my $func = $1;
> + my $func_capacity = "NEEDLESS_$1";
> +
> + $func_capacity =~ s/(.$)/\U$1\E/;
> +
> + WARN($func_capacity,
> + "$func(NULL) is safe this check is probably not required\n" . $hereprev);
Perhaps just
WARN("NEEDLESS_IF",
...
On Tue, Nov 20, 2012 at 06:43:49AM -0800, Joe Perches wrote:
> On Tue, 2012-11-20 at 14:29 +0000, Andy Whitcroft wrote:
>
> > This all looks sensible, though we still have three blocks doing the
> > same thing. How about we standardise this check into a single check,
> > generating the capacity from the matched name.
> []
> > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> []
> > + if ($line =~ /\b(kfree|usb_free_urb|debugfs_remove(?:_recursive)?)$expr/) {
> > + my $func = $1;
> > + my $func_capacity = "NEEDLESS_$1";
> > +
> > + $func_capacity =~ s/(.$)/\U$1\E/;
> > +
> > + WARN($func_capacity,
> > + "$func(NULL) is safe this check is probably not required\n" . $hereprev);
>
> Perhaps just
> WARN("NEEDLESS_IF",
I would cirtainly be happy with that, I was trying to avoid changing the
capacity for the existing NEEDLESS_KFREE. If compatibility there isn't
an issue then that makes life even simpler.
-apw
>> On Tue, 2012-11-20 at 14:29 +0000, Andy Whitcroft wrote:
>> Perhaps just
>> WARN("NEEDLESS_IF",
>
> I would cirtainly be happy with that, I was trying to avoid changing the
> capacity for the existing NEEDLESS_KFREE. If compatibility there isn't
> an issue then that makes life even simpler.
>
> -apw
Right.
On Tue, 2012-11-20 at 16:50 +0200, Constantine Shulyupin wrote:
> >> On Tue, 2012-11-20 at 14:29 +0000, Andy Whitcroft wrote:
> >> Perhaps just
> >> WARN("NEEDLESS_IF",
> >
> > I would cirtainly be happy with that, I was trying to avoid changing the
> > capacity for the existing NEEDLESS_KFREE. If compatibility there isn't
> > an issue then that makes life even simpler.
> >
> > -apw
>
> Right.
I don't think it's an issue. I'm not sure anyone
really uses --ignore for much other than LONG_LINE.
On Tue, Nov 20, 2012 at 4:58 PM, Joe Perches <[email protected]> wrote:
> I don't think it's an issue. I'm not sure anyone
> really uses --ignore for much other than LONG_LINE.
Indeed. Must all code be 80 characters width? My be my be better to
make line length configurable than ignore it? Or at least print actual
line length to allow developer be aware how long lines is.
I think 90 char line is preferable than wrapped two or even three times line.
Thanks
On Tue, Nov 20, 2012 at 05:10:43PM +0200, Constantine Shulyupin wrote:
> On Tue, Nov 20, 2012 at 4:58 PM, Joe Perches <[email protected]> wrote:
> > I don't think it's an issue. I'm not sure anyone
> > really uses --ignore for much other than LONG_LINE.
>
> Indeed. Must all code be 80 characters width? My be my be better to
> make line length configurable than ignore it? Or at least print actual
> line length to allow developer be aware how long lines is.
> I think 90 char line is preferable than wrapped two or even three times line.
If wrapping there is that much better then wrapping there is appropriate
in some cases. The currently limit has been revisited a bunch of times,
and in none have we agreed on what to change it to. It therefore
remains the recommendation.
-apw
Consolidate the if (foo) bar(foo) detectors into a single check. Add
debugfs_remove and family.
Based on a patch by Constantine Shulyupin <[email protected]>.
Signed-off-by: Andy Whitcroft <[email protected]>
---
scripts/checkpatch.pl | 20 ++++++++------------
1 file changed, 8 insertions(+), 12 deletions(-)
This is a fully merged version as the fix patch was bigger than the
whole merged patch and this is much clearer as to purpose. Hope that
makes sense.
-apw
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index ae01b90..e83a137 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -3239,20 +3239,16 @@ sub process {
$herecurr);
}
+# check for needless "if (<foo>) fn(<foo>)" uses
+ if ($prevline =~ /\bif\s*\(\s*($Lval)\s*\)/) {
+ my $expr = '\s*\(\s*' . quotemeta($1) . '\s*\)\s*;';
+
# check for needless kfree() checks
- if ($prevline =~ /\bif\s*\(([^\)]*)\)/) {
- my $expr = $1;
- if ($line =~ /\bkfree\(\Q$expr\E\);/) {
- WARN("NEEDLESS_KFREE",
- "kfree(NULL) is safe this check is probably not required\n" . $hereprev);
- }
- }
# check for needless usb_free_urb() checks
- if ($prevline =~ /\bif\s*\(([^\)]*)\)/) {
- my $expr = $1;
- if ($line =~ /\busb_free_urb\(\Q$expr\E\);/) {
- WARN("NEEDLESS_USB_FREE_URB",
- "usb_free_urb(NULL) is safe this check is probably not required\n" . $hereprev);
+# check for needless debugfs_remove() and debugfs_remove_recursive*() checks
+ if ($line =~ /\b(kfree|usb_free_urb|debugfs_remove(?:_recursive)?)$expr/) {
+ WARN('NEEDLESS_IF',
+ "$1(NULL) is safe this check is probably not required\n" . $hereprev);
}
}
--
1.7.10.4
On Tue, 2012-11-20 at 15:37 +0000, Andy Whitcroft wrote:
> Consolidate the if (foo) bar(foo) detectors into a single check. Add
> debugfs_remove and family.
>
> Based on a patch by Constantine Shulyupin <[email protected]>.
[]
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
[]
>
> +# check for needless "if (<foo>) fn(<foo>)" uses
> + if ($prevline =~ /\bif\s*\(\s*($Lval)\s*\)/) {
> + my $expr = '\s*\(\s*' . quotemeta($1) . '\s*\)\s*;';
> +
[]
> +# check for needless debugfs_remove() and debugfs_remove_recursive*() checks
Hey Andy, that's an incomplete comment.
Just remove it.
> + if ($line =~ /\b(kfree|usb_free_urb|debugfs_remove(?:_recursive)?)$expr/) {
> + WARN('NEEDLESS_IF',
> + "$1(NULL) is safe this check is probably not required\n" . $hereprev);
> }
> }
>
On Tue, Nov 20, 2012 at 07:51:17AM -0800, Joe Perches wrote:
> On Tue, 2012-11-20 at 15:37 +0000, Andy Whitcroft wrote:
> > Consolidate the if (foo) bar(foo) detectors into a single check. Add
> > debugfs_remove and family.
> >
> > Based on a patch by Constantine Shulyupin <[email protected]>.
> []
> > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> []
> >
> > +# check for needless "if (<foo>) fn(<foo>)" uses
> > + if ($prevline =~ /\bif\s*\(\s*($Lval)\s*\)/) {
> > + my $expr = '\s*\(\s*' . quotemeta($1) . '\s*\)\s*;';
> > +
> []
> > +# check for needless debugfs_remove() and debugfs_remove_recursive*() checks
>
> Hey Andy, that's an incomplete comment.
> Just remove it.
Oh it is meant to drop next to the other comments from the preceeding
hunks which are being removed, it should end up looking like this:
# check for needless kfree() checks
# check for needless usb_free_urb() checks
# check for needless debugfs_remove() and debugfs_remove_recursive*() checks
Admitedly the trailing checks on each are a little redundant, but it is
intended to retain the list of functions affected.
>
> > + if ($line =~ /\b(kfree|usb_free_urb|debugfs_remove(?:_recursive)?)$expr/) {
> > + WARN('NEEDLESS_IF',
> > + "$1(NULL) is safe this check is probably not required\n" . $hereprev);
> > }
> > }
> >
-apw
On Tue, 2012-11-20 at 16:39 +0000, Andy Whitcroft wrote:
> On Tue, Nov 20, 2012 at 07:51:17AM -0800, Joe Perches wrote:
> > On Tue, 2012-11-20 at 15:37 +0000, Andy Whitcroft wrote:
> > > Consolidate the if (foo) bar(foo) detectors into a single check. Add
> > > debugfs_remove and family.
> > >
> > > Based on a patch by Constantine Shulyupin <[email protected]>.
> > []
> > > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> > []
> > >
> > > +# check for needless "if (<foo>) fn(<foo>)" uses
> > > + if ($prevline =~ /\bif\s*\(\s*($Lval)\s*\)/) {
> > > + my $expr = '\s*\(\s*' . quotemeta($1) . '\s*\)\s*;';
> > > +
> > []
> > > +# check for needless debugfs_remove() and debugfs_remove_recursive*() checks
> >
> > Hey Andy, that's an incomplete comment.
> > Just remove it.
>
> Oh it is meant to drop next to the other comments from the preceeding
> hunks which are being removed, it should end up looking like this:
>
> # check for needless kfree() checks
> # check for needless usb_free_urb() checks
> # check for needless debugfs_remove() and debugfs_remove_recursive*() checks
>
> Admitedly the trailing checks on each are a little redundant, but it is
> intended to retain the list of functions affected.
I think all of those are unnecessary.
Self documenting code is better right?
If not, I'd remove the "check for needless", leave the function,
and remove the trailing "checks"
btw: the * after debugfs_remove_recursive should be removed too.
cheers, Joe
Consolidate the if (foo) bar(foo) detectors into a single check. Add
debugfs_remove and family.
Based on a patch by Constantine Shulyupin <[email protected]>.
Signed-off-by: Andy Whitcroft <[email protected]>
---
scripts/checkpatch.pl | 20 ++++++--------------
1 file changed, 6 insertions(+), 14 deletions(-)
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index ae01b90..a1e2471 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -3239,20 +3239,12 @@ sub process {
$herecurr);
}
-# check for needless kfree() checks
- if ($prevline =~ /\bif\s*\(([^\)]*)\)/) {
- my $expr = $1;
- if ($line =~ /\bkfree\(\Q$expr\E\);/) {
- WARN("NEEDLESS_KFREE",
- "kfree(NULL) is safe this check is probably not required\n" . $hereprev);
- }
- }
-# check for needless usb_free_urb() checks
- if ($prevline =~ /\bif\s*\(([^\)]*)\)/) {
- my $expr = $1;
- if ($line =~ /\busb_free_urb\(\Q$expr\E\);/) {
- WARN("NEEDLESS_USB_FREE_URB",
- "usb_free_urb(NULL) is safe this check is probably not required\n" . $hereprev);
+# check for needless "if (<foo>) fn(<foo>)" uses
+ if ($prevline =~ /\bif\s*\(\s*($Lval)\s*\)/) {
+ my $expr = '\s*\(\s*' . quotemeta($1) . '\s*\)\s*;';
+ if ($line =~ /\b(kfree|usb_free_urb|debugfs_remove(?:_recursive)?)$expr/) {
+ WARN('NEEDLESS_IF',
+ "$1(NULL) is safe this check is probably not required\n" . $hereprev);
}
}
--
1.7.10.4
Some projects might want a longer line length so allow
a command line --max-line-length=n control over the
long line warnings. The default line length is 80.
Signed-off-by: Joe Perches <[email protected]>
---
scripts/checkpatch.pl | 9 ++++++---
1 files changed, 6 insertions(+), 3 deletions(-)
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index d2d5ba1..38e8890 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -33,6 +33,7 @@ my %ignore_type = ();
my @ignore = ();
my $help = 0;
my $configuration_file = ".checkpatch.conf";
+my $max_line_length = 80;
sub help {
my ($exitcode) = @_;
@@ -51,6 +52,7 @@ Options:
-f, --file treat FILE as regular source file
--subjective, --strict enable more subjective tests
--ignore TYPE(,TYPE2...) ignore various comma separated message types
+ --max-line-length=n set the maximum line length, if exceeded, warn
--show-types show the message "types" in the output
--root=PATH PATH to the kernel tree root
--no-summary suppress the per-file summary
@@ -107,6 +109,7 @@ GetOptions(
'strict!' => \$check,
'ignore=s' => \@ignore,
'show-types!' => \$show_types,
+ 'max-line-length=i' => \$max_line_length,
'root=s' => \$root,
'summary!' => \$summary,
'mailback!' => \$mailback,
@@ -1760,15 +1763,15 @@ sub process {
# check we are in a valid source file if not then ignore this hunk
next if ($realfile !~ /\.(h|c|s|S|pl|sh)$/);
-#80 column limit
+#line length limit
if ($line =~ /^\+/ && $prevrawline !~ /\/\*\*/ &&
$rawline !~ /^.\s*\*\s*\@$Ident\s/ &&
!($line =~ /^\+\s*$logFunctions\s*\(\s*(?:(KERN_\S+\s*|[^"]*))?"[X\t]*"\s*(?:|,|\)\s*;)\s*$/ ||
$line =~ /^\+\s*"[^"]*"\s*(?:\s*|,|\)\s*;)\s*$/) &&
- $length > 80)
+ $length > $max_line_length)
{
WARN("LONG_LINE",
- "line over 80 characters\n" . $herecurr);
+ "line over $max_line_length characters\n" . $herecurr);
}
# Check for user-visible strings broken across lines, which breaks the ability