checkpatch: add --strict "pointer comparison to NULL" test
Joe Perches
maintainerPATCH 1/1 checkpatch: add --strict "pointer comparison to NULL" test
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.
---
Seeing no objections, so submitting it.
scripts/checkpatch.pl | 14 ++++++++++++++ 1 file changed, 14 insertions(+)
scripts/checkpatch.pl +14 -0
@@ -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;
Quoted 27 lines · Joe Perches
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 <[email protected]>---
Seeing no objections, so submitting it.scripts/checkpatch.pl | 14 ++++++++++++++ 1 file changed, 14 insertions(+)scripts/checkpatch.pl +4
-0@@ -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;
scripts/checkpatch.pl sub process { · line 4506
Quoted 10 lines
scripts/checkpatch.pl +10
-0+ 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
Joe Perches
maintainerQuoted 4 lines · Viresh Kumar
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(+)
scripts/checkpatch.pl +23 -0
@@ -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;
----- Original Message -----
Quoted 49 lines
From: "Joe Perches" <[email protected]>
To: "Viresh Kumar" <[email protected]>
Cc: "Andrew Morton" <[email protected]>, "Dan Carpenter" <[email protected]>, "Greg KH" <[email protected]>, "LKML" <[email protected]>, "Mike Holmes" <[email protected]>, [email protected]
Sent: Thursday, 27 August, 2015 5:05:37 AM
Subject: Re: [PATCH] checkpatch: add --strict "pointer comparison to NULL" testQuoted 4 lines · Viresh Kumar
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 < barshould probably be a separate test.
Something like:
--- scripts/checkpatch.pl | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+)scripts/checkpatch.pl +9
-0@@ -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) &&
scripts/checkpatch.pl sub process { · line 4252
Quoted 10 lines
scripts/checkpatch.pl +10
-0+ $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
Joe Perches
maintainerQuoted 1 line · Nicolas Morey Chaisemartin
Quoted 1 line
From: "Joe Perches" <[email protected]>
[]
Quoted 6 lines
Quoted 6 lines
And because most likely, "CONST test variable" checks like
NULL != foo and 0 < barshould probably be a separate test.
[]
Quoted 1 line
scripts/checkpatch.pl
Quoted 1 line
scripts/checkpatch.pl +0
-0
[]
Quoted 4 lines
Quoted 4 lines
scripts/checkpatch.pl +4
-0+ if ($comp eq "<") { + $newcomp = ">="; + } elsif ($comp eq "<=") { + $newcomp = ">";
Quoted 8 lines
scripts/checkpatch.pl sub process { · line 4252
Quoted 5 lines
scripts/checkpatch.pl +5
-0+ } 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.
Joe Perches
maintainerPATCH 1/1 checkpatch: add constant comparison on left side test
"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)"
--- scripts/checkpatch.pl | 29 +++++++++++++++++++++++++++++ 1 file changed, 29 insertions(+)
scripts/checkpatch.pl +29 -0
@@ -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;