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