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