* [PATCH] checkpatch: add --strict "pointer comparison to NULL" test @ 2014-11-13 18:57 Joe Perches 2015-08-27 2:19 ` Viresh Kumar 0 siblings, 1 reply; 6+ messages in thread From: Joe Perches @ 2014-11-13 18:57 UTC (permalink / raw) To: Andrew Morton; +Cc: Dan Carpenter, Greg KH, LKML It seems there are more and more uses of "if (!ptr)" in preference to "if (ptr == NULL)" so add a --strict test to emit a message when using the latter form. This also finds (ptr != NULL). Fix it too if desired. Signed-off-by: Joe Perches <joe@perches.com> --- Seeing no objections, so submitting it. scripts/checkpatch.pl | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl index 24d6702..d4e08bc 100755 --- a/scripts/checkpatch.pl +++ b/scripts/checkpatch.pl @@ -4490,6 +4490,20 @@ sub process { "Possible precedence defect with mask then right shift - may need parentheses\n" . $herecurr); } +# check for pointer comparisons to NULL + if ($^V && $^V ge 5.10.0) { + while ($line =~ /\b$LvalOrFunc\s*(==|\!=)\s*NULL\b/g) { + my $val = $1; + my $equal = "!"; + $equal = "" if ($4 eq "!="); + if (CHK("COMPARISON_TO_NULL", + "Comparison to NULL could be written \"${equal}${val}\"\n" . $herecurr) && + $fix) { + $fixed[$fixlinenr] =~ s/\b\Q$val\E\s*(?:==|\!=)\s*NULL\b/$equal$val/; + } + } + } + # check for bad placement of section $InitAttribute (e.g.: __initdata) if ($line =~ /(\b$InitAttribute\b)/) { my $attr = $1; ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] checkpatch: add --strict "pointer comparison to NULL" test 2014-11-13 18:57 [PATCH] checkpatch: add --strict "pointer comparison to NULL" test Joe Perches @ 2015-08-27 2:19 ` Viresh Kumar 2015-08-27 3:05 ` Joe Perches 0 siblings, 1 reply; 6+ messages in thread From: Viresh Kumar @ 2015-08-27 2:19 UTC (permalink / raw) To: Joe Perches Cc: Andrew Morton, Dan Carpenter, Greg KH, LKML, Mike Holmes, nmorey On Fri, Nov 14, 2014 at 12:27 AM, Joe Perches <joe@perches.com> wrote: > It seems there are more and more uses of "if (!ptr)" > in preference to "if (ptr == NULL)" so add a --strict > test to emit a message when using the latter form. > > This also finds (ptr != NULL). > > Fix it too if desired. > > Signed-off-by: Joe Perches <joe@perches.com> > --- > Seeing no objections, so submitting it. > > scripts/checkpatch.pl | 14 ++++++++++++++ > 1 file changed, 14 insertions(+) > > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl > index 24d6702..d4e08bc 100755 > --- a/scripts/checkpatch.pl > +++ b/scripts/checkpatch.pl > @@ -4490,6 +4490,20 @@ sub process { > "Possible precedence defect with mask then right shift - may need parentheses\n" . $herecurr); > } > > +# check for pointer comparisons to NULL > + if ($^V && $^V ge 5.10.0) { > + while ($line =~ /\b$LvalOrFunc\s*(==|\!=)\s*NULL\b/g) { > + my $val = $1; > + my $equal = "!"; > + $equal = "" if ($4 eq "!="); > + if (CHK("COMPARISON_TO_NULL", > + "Comparison to NULL could be written \"${equal}${val}\"\n" . $herecurr) && > + $fix) { > + $fixed[$fixlinenr] =~ s/\b\Q$val\E\s*(?:==|\!=)\s*NULL\b/$equal$val/; > + } > + } > + } > + Hi Joe, Sorry for picking a relatively old thread. Few colleagues asked me why isn't checkpatch warning for (NULL == ptr) or (NULL != ptr) checks, as it warns for (ptr == NULL) and (ptr != NULL). Did you miss it? or was it intentional ? -- viresh ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] checkpatch: add --strict "pointer comparison to NULL" test 2015-08-27 2:19 ` Viresh Kumar @ 2015-08-27 3:05 ` Joe Perches 2015-08-27 7:09 ` Nicolas Morey Chaisemartin 0 siblings, 1 reply; 6+ messages in thread From: Joe Perches @ 2015-08-27 3:05 UTC (permalink / raw) To: Viresh Kumar Cc: Andrew Morton, Dan Carpenter, Greg KH, LKML, Mike Holmes, nmorey On Thu, 2015-08-27 at 07:49 +0530, Viresh Kumar wrote: > Few colleagues asked me why isn't checkpatch warning for (NULL == ptr) > or (NULL != ptr) checks, as it warns for (ptr == NULL) and (ptr != NULL). > > Did you miss it? or was it intentional ? I didn't miss it. NULL == foo is relatively unusual and not really worth the bother. And because most likely, "CONST test variable" checks like NULL != foo and 0 < bar should probably be a separate test. Something like: --- scripts/checkpatch.pl | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl index e14dcdb..457ddef 100755 --- a/scripts/checkpatch.pl +++ b/scripts/checkpatch.pl @@ -4231,6 +4231,29 @@ sub process { } } +# comparisons with a constant on the left + if ($^V && $^V ge 5.10.0 && + $line =~ /\b($Constant|[A-Z_]+)\s*($Compare)\s*($LvalOrFunc)/) { + my $const = $1; + my $comp = $2; + my $to = $3; + my $newcomp = $comp; + if (WARN("CONSTANT_COMPARISON", + "Comparisons should place the constant on the right side of the test\n" . $herecurr) && + $fix) { + if ($comp eq "<") { + $newcomp = ">="; + } elsif ($comp eq "<=") { + $newcomp = ">"; + } elsif ($comp eq ">") { + $newcomp = "<="; + } elsif ($comp eq ">=") { + $newcomp = "<"; + } + $fixed[$fixlinenr] =~ s/\(\s*\Q$const\E\s*$Compare\s*\Q$to\E\s*\)/($to $newcomp $const)/; + } + } + # Return of what appears to be an errno should normally be negative if ($sline =~ /\breturn(?:\s*\(+\s*|\s+)(E[A-Z]+)(?:\s*\)+\s*|\s*)[;:,]/) { my $name = $1; ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] checkpatch: add --strict "pointer comparison to NULL" test 2015-08-27 3:05 ` Joe Perches @ 2015-08-27 7:09 ` Nicolas Morey Chaisemartin 2015-08-27 7:22 ` Joe Perches 0 siblings, 1 reply; 6+ messages in thread From: Nicolas Morey Chaisemartin @ 2015-08-27 7:09 UTC (permalink / raw) To: Joe Perches Cc: Viresh Kumar, Andrew Morton, Dan Carpenter, Greg KH, LKML, Mike Holmes ----- Original Message ----- > From: "Joe Perches" <joe@perches.com> > To: "Viresh Kumar" <viresh.kumar@linaro.org> > Cc: "Andrew Morton" <akpm@linux-foundation.org>, "Dan Carpenter" <dan.carpenter@oracle.com>, "Greg KH" > <gregkh@linuxfoundation.org>, "LKML" <linux-kernel@vger.kernel.org>, "Mike Holmes" <mike.holmes@linaro.org>, > nmorey@kalray.eu > Sent: Thursday, 27 August, 2015 5:05:37 AM > Subject: Re: [PATCH] checkpatch: add --strict "pointer comparison to NULL" test > > On Thu, 2015-08-27 at 07:49 +0530, Viresh Kumar wrote: > > Few colleagues asked me why isn't checkpatch warning for (NULL == ptr) > > or (NULL != ptr) checks, as it warns for (ptr == NULL) and (ptr != NULL). > > > > Did you miss it? or was it intentional ? > > I didn't miss it. > > NULL == foo is relatively unusual and not really worth the > bother. > > And because most likely, "CONST test variable" checks like > NULL != foo > and > 0 < bar > > should probably be a separate test. > > Something like: > --- > scripts/checkpatch.pl | 23 +++++++++++++++++++++++ > 1 file changed, 23 insertions(+) > > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl > index e14dcdb..457ddef 100755 > --- a/scripts/checkpatch.pl > +++ b/scripts/checkpatch.pl > @@ -4231,6 +4231,29 @@ sub process { > } > } > > +# comparisons with a constant on the left > + if ($^V && $^V ge 5.10.0 && > + $line =~ /\b($Constant|[A-Z_]+)\s*($Compare)\s*($LvalOrFunc)/) { > + my $const = $1; > + my $comp = $2; > + my $to = $3; > + my $newcomp = $comp; > + if (WARN("CONSTANT_COMPARISON", > + "Comparisons should place the constant on the right side of the test\n" > . $herecurr) && > + $fix) { > + if ($comp eq "<") { > + $newcomp = ">="; > + } elsif ($comp eq "<=") { > + $newcomp = ">"; > + } elsif ($comp eq ">") { > + $newcomp = "<="; > + } elsif ($comp eq ">=") { > + $newcomp = "<"; > + } I like the concept but are you sure about this? I think the "=" should be added or removed. If a < b, b > a, not b >= a. Nicolas ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] checkpatch: add --strict "pointer comparison to NULL" test 2015-08-27 7:09 ` Nicolas Morey Chaisemartin @ 2015-08-27 7:22 ` Joe Perches 2015-08-27 17:33 ` [PATCH] checkpatch: add constant comparison on left side test Joe Perches 0 siblings, 1 reply; 6+ messages in thread From: Joe Perches @ 2015-08-27 7:22 UTC (permalink / raw) To: Nicolas Morey Chaisemartin Cc: Viresh Kumar, Andrew Morton, Dan Carpenter, Greg KH, LKML, Mike Holmes On Thu, 2015-08-27 at 09:09 +0200, Nicolas Morey Chaisemartin wrote: > > From: "Joe Perches" <joe@perches.com> [] > > And because most likely, "CONST test variable" checks like > > NULL != foo > > and > > 0 < bar > > > > should probably be a separate test. [] > > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl [] > > + if ($comp eq "<") { > > + $newcomp = ">="; > > + } elsif ($comp eq "<=") { > > + $newcomp = ">"; > > + } elsif ($comp eq ">") { > > + $newcomp = "<="; > > + } elsif ($comp eq ">=") { > > + $newcomp = "<"; > > + } > > I like the concept but are you sure about this? > I think the "=" should be added or removed. If a < b, b > a, not b >= a. Right thanks, it's obviously incorrect. ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH] checkpatch: add constant comparison on left side test 2015-08-27 7:22 ` Joe Perches @ 2015-08-27 17:33 ` Joe Perches 0 siblings, 0 replies; 6+ messages in thread From: Joe Perches @ 2015-08-27 17:33 UTC (permalink / raw) To: Andrew Morton Cc: Nicolas Morey Chaisemartin, Viresh Kumar, Dan Carpenter, LKML "CONST <comparison> variable" checks like: if (NULL != foo) and while (0 < bar(...)) where a constant (or what appears to be a constant like an upper case identifier) is on the left of a comparison are generally preferred to be written using the constant on the right side like: if (foo != NULL) and while (bar(...) > 0) Add a test for this. Add a --fix option too, but only do it when the code is immediately surrounded by parentheses to avoid misfixing things like "(0 < bar() + constant)" Signed-off-by: Joe Perches <joe@perches.com> --- scripts/checkpatch.pl | 29 +++++++++++++++++++++++++++++ 1 file changed, 29 insertions(+) diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl index e14dcdb..5fca1da 100755 --- a/scripts/checkpatch.pl +++ b/scripts/checkpatch.pl @@ -4231,6 +4231,35 @@ sub process { } } +# comparisons with a constant or upper case identifier on the left +# avoid cases like "foo + BAR < baz" +# only fix matches surrounded by parentheses to avoid incorrect +# conversions like "FOO < baz() + 5" being "misfixed" to "baz() > FOO + 5" + if ($^V && $^V ge 5.10.0 && + $line =~ /^\+(.*)\b($Constant|[A-Z_][A-Z0-9_]*)\s*($Compare)\s*($LvalOrFunc)/) { + my $lead = $1; + my $const = $2; + my $comp = $3; + my $to = $4; + my $newcomp = $comp; + if ($lead !~ /$Operators\s*$/ && + $to !~ /^(?:Constant|[A-Z_][A-Z0-9_]*)$/ && + WARN("CONSTANT_COMPARISON", + "Comparisons should place the constant on the right side of the test\n" . $herecurr) && + $fix) { + if ($comp eq "<") { + $newcomp = ">"; + } elsif ($comp eq "<=") { + $newcomp = ">="; + } elsif ($comp eq ">") { + $newcomp = "<"; + } elsif ($comp eq ">=") { + $newcomp = "<="; + } + $fixed[$fixlinenr] =~ s/\(\s*\Q$const\E\s*$Compare\s*\Q$to\E\s*\)/($to $newcomp $const)/; + } + } + # Return of what appears to be an errno should normally be negative if ($sline =~ /\breturn(?:\s*\(+\s*|\s+)(E[A-Z]+)(?:\s*\)+\s*|\s*)[;:,]/) { my $name = $1; ^ permalink raw reply related [flat|nested] 6+ messages in thread
end of thread, other threads:[~2015-08-27 17:33 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2014-11-13 18:57 [PATCH] checkpatch: add --strict "pointer comparison to NULL" test Joe Perches 2015-08-27 2:19 ` Viresh Kumar 2015-08-27 3:05 ` Joe Perches 2015-08-27 7:09 ` Nicolas Morey Chaisemartin 2015-08-27 7:22 ` Joe Perches 2015-08-27 17:33 ` [PATCH] checkpatch: add constant comparison on left side test Joe Perches
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox