* [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 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.