* [PATCH v4] checkpatch: debugfs_remove() can take NULL @ 2012-11-17 22:20 Constantine Shulyupin 2012-11-20 14:29 ` Andy Whitcroft 0 siblings, 1 reply; 14+ messages in thread From: Constantine Shulyupin @ 2012-11-17 22:20 UTC (permalink / raw) To: linux-kernel, gregkh, Andy Whitcroft, joe; +Cc: Constantine Shulyupin From: Constantine Shulyupin <const@MakeLinux.com> 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 <const@MakeLinux.com> --- 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 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH v4] checkpatch: debugfs_remove() can take NULL 2012-11-17 22:20 [PATCH v4] checkpatch: debugfs_remove() can take NULL Constantine Shulyupin @ 2012-11-20 14:29 ` Andy Whitcroft 2012-11-20 14:43 ` Joe Perches 0 siblings, 1 reply; 14+ messages in thread From: Andy Whitcroft @ 2012-11-20 14:29 UTC (permalink / raw) To: Constantine Shulyupin; +Cc: linux-kernel, gregkh, joe On Sun, Nov 18, 2012 at 12:20:18AM +0200, Constantine Shulyupin wrote: > From: Constantine Shulyupin <const@MakeLinux.com> > > 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 <const@MakeLinux.com> > --- > 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 <apw@canonical.com> Date: Tue, 20 Nov 2012 14:27:59 +0000 Subject: [PATCH] checkpatch: consolidate if (foo) bar(NULL) checks Signed-off-by: Andy Whitcroft <apw@canonical.com> --- 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 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH v4] checkpatch: debugfs_remove() can take NULL 2012-11-20 14:29 ` Andy Whitcroft @ 2012-11-20 14:43 ` Joe Perches 2012-11-20 14:47 ` Andy Whitcroft 0 siblings, 1 reply; 14+ messages in thread From: Joe Perches @ 2012-11-20 14:43 UTC (permalink / raw) To: Andy Whitcroft; +Cc: Constantine Shulyupin, linux-kernel, gregkh 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", ... ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v4] checkpatch: debugfs_remove() can take NULL 2012-11-20 14:43 ` Joe Perches @ 2012-11-20 14:47 ` Andy Whitcroft 2012-11-20 14:50 ` Constantine Shulyupin 0 siblings, 1 reply; 14+ messages in thread From: Andy Whitcroft @ 2012-11-20 14:47 UTC (permalink / raw) To: Joe Perches; +Cc: Constantine Shulyupin, linux-kernel, gregkh 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v4] checkpatch: debugfs_remove() can take NULL 2012-11-20 14:47 ` Andy Whitcroft @ 2012-11-20 14:50 ` Constantine Shulyupin 2012-11-20 14:58 ` Joe Perches 0 siblings, 1 reply; 14+ messages in thread From: Constantine Shulyupin @ 2012-11-20 14:50 UTC (permalink / raw) To: Andy Whitcroft; +Cc: Joe Perches, linux-kernel, gregkh >> 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. ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v4] checkpatch: debugfs_remove() can take NULL 2012-11-20 14:50 ` Constantine Shulyupin @ 2012-11-20 14:58 ` Joe Perches 2012-11-20 15:10 ` Constantine Shulyupin 2012-11-20 15:37 ` [PATCH] checkpatch: consolidate if (foo) bar(foo) checks and add debugfs_remove Andy Whitcroft 0 siblings, 2 replies; 14+ messages in thread From: Joe Perches @ 2012-11-20 14:58 UTC (permalink / raw) To: Constantine Shulyupin; +Cc: Andy Whitcroft, linux-kernel, gregkh 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. ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v4] checkpatch: debugfs_remove() can take NULL 2012-11-20 14:58 ` Joe Perches @ 2012-11-20 15:10 ` Constantine Shulyupin 2012-11-20 15:22 ` Andy Whitcroft 2012-11-20 15:37 ` [PATCH] checkpatch: consolidate if (foo) bar(foo) checks and add debugfs_remove Andy Whitcroft 1 sibling, 1 reply; 14+ messages in thread From: Constantine Shulyupin @ 2012-11-20 15:10 UTC (permalink / raw) To: Joe Perches; +Cc: Andy Whitcroft, linux-kernel, gregkh On Tue, Nov 20, 2012 at 4:58 PM, Joe Perches <joe@perches.com> 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v4] checkpatch: debugfs_remove() can take NULL 2012-11-20 15:10 ` Constantine Shulyupin @ 2012-11-20 15:22 ` Andy Whitcroft 2012-11-20 20:57 ` [PATCH] checkpatch: Allow control over line length warning, default remains 80 Joe Perches 0 siblings, 1 reply; 14+ messages in thread From: Andy Whitcroft @ 2012-11-20 15:22 UTC (permalink / raw) To: Constantine Shulyupin; +Cc: Joe Perches, linux-kernel, gregkh On Tue, Nov 20, 2012 at 05:10:43PM +0200, Constantine Shulyupin wrote: > On Tue, Nov 20, 2012 at 4:58 PM, Joe Perches <joe@perches.com> 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH] checkpatch: Allow control over line length warning, default remains 80 2012-11-20 15:22 ` Andy Whitcroft @ 2012-11-20 20:57 ` Joe Perches 0 siblings, 0 replies; 14+ messages in thread From: Joe Perches @ 2012-11-20 20:57 UTC (permalink / raw) To: Andy Whitcroft, Andrew Morton; +Cc: Constantine Shulyupin, linux-kernel, gregkh 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 <joe@perches.com> --- 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 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH] checkpatch: consolidate if (foo) bar(foo) checks and add debugfs_remove 2012-11-20 14:58 ` Joe Perches 2012-11-20 15:10 ` Constantine Shulyupin @ 2012-11-20 15:37 ` Andy Whitcroft 2012-11-20 15:51 ` Joe Perches 1 sibling, 1 reply; 14+ messages in thread From: Andy Whitcroft @ 2012-11-20 15:37 UTC (permalink / raw) To: Joe Perches; +Cc: Constantine Shulyupin, linux-kernel, gregkh Consolidate the if (foo) bar(foo) detectors into a single check. Add debugfs_remove and family. Based on a patch by Constantine Shulyupin <const@MakeLinux.com>. Signed-off-by: Andy Whitcroft <apw@canonical.com> --- 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 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH] checkpatch: consolidate if (foo) bar(foo) checks and add debugfs_remove 2012-11-20 15:37 ` [PATCH] checkpatch: consolidate if (foo) bar(foo) checks and add debugfs_remove Andy Whitcroft @ 2012-11-20 15:51 ` Joe Perches 2012-11-20 16:39 ` Andy Whitcroft 0 siblings, 1 reply; 14+ messages in thread From: Joe Perches @ 2012-11-20 15:51 UTC (permalink / raw) To: Andy Whitcroft; +Cc: Constantine Shulyupin, linux-kernel, gregkh 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 <const@MakeLinux.com>. [] > 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); > } > } > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] checkpatch: consolidate if (foo) bar(foo) checks and add debugfs_remove 2012-11-20 15:51 ` Joe Perches @ 2012-11-20 16:39 ` Andy Whitcroft 2012-11-20 18:37 ` Joe Perches 0 siblings, 1 reply; 14+ messages in thread From: Andy Whitcroft @ 2012-11-20 16:39 UTC (permalink / raw) To: Joe Perches; +Cc: Constantine Shulyupin, linux-kernel, gregkh 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 <const@MakeLinux.com>. > [] > > 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] checkpatch: consolidate if (foo) bar(foo) checks and add debugfs_remove 2012-11-20 16:39 ` Andy Whitcroft @ 2012-11-20 18:37 ` Joe Perches 2012-11-20 19:17 ` [PATCH V2] " Andy Whitcroft 0 siblings, 1 reply; 14+ messages in thread From: Joe Perches @ 2012-11-20 18:37 UTC (permalink / raw) To: Andy Whitcroft; +Cc: Constantine Shulyupin, linux-kernel, gregkh 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 <const@MakeLinux.com>. > > [] > > > 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH V2] checkpatch: consolidate if (foo) bar(foo) checks and add debugfs_remove 2012-11-20 18:37 ` Joe Perches @ 2012-11-20 19:17 ` Andy Whitcroft 0 siblings, 0 replies; 14+ messages in thread From: Andy Whitcroft @ 2012-11-20 19:17 UTC (permalink / raw) To: Joe Perches; +Cc: Constantine Shulyupin, linux-kernel, gregkh, Andrew Morton Consolidate the if (foo) bar(foo) detectors into a single check. Add debugfs_remove and family. Based on a patch by Constantine Shulyupin <const@MakeLinux.com>. Signed-off-by: Andy Whitcroft <apw@canonical.com> --- 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 ^ permalink raw reply related [flat|nested] 14+ messages in thread
end of thread, other threads:[~2012-11-20 20:57 UTC | newest] Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2012-11-17 22:20 [PATCH v4] checkpatch: debugfs_remove() can take NULL Constantine Shulyupin 2012-11-20 14:29 ` Andy Whitcroft 2012-11-20 14:43 ` Joe Perches 2012-11-20 14:47 ` Andy Whitcroft 2012-11-20 14:50 ` Constantine Shulyupin 2012-11-20 14:58 ` Joe Perches 2012-11-20 15:10 ` Constantine Shulyupin 2012-11-20 15:22 ` Andy Whitcroft 2012-11-20 20:57 ` [PATCH] checkpatch: Allow control over line length warning, default remains 80 Joe Perches 2012-11-20 15:37 ` [PATCH] checkpatch: consolidate if (foo) bar(foo) checks and add debugfs_remove Andy Whitcroft 2012-11-20 15:51 ` Joe Perches 2012-11-20 16:39 ` Andy Whitcroft 2012-11-20 18:37 ` Joe Perches 2012-11-20 19:17 ` [PATCH V2] " Andy Whitcroft
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.