Comparisons of A to true and false are better written
as A and !A.
Bleat a message on use.
Signed-off-by: Joe Perches <[email protected]>
---
scripts/checkpatch.pl | 17 +++++++++++++++++
1 file changed, 17 insertions(+)
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 3fb6d86..080e7f6 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -3538,6 +3538,23 @@ sub process {
"Using yield() is generally wrong. See yield() kernel-doc (sched/core.c)\n" . $herecurr);
}
+# check for comparisons against true and false
+ if ($line =~ /\+\s*(.*?)($Lval)\s*(==|\!=)\s*(true|false)\b(.*)$/i) {
+ my $lead = $1;
+ my $arg = $2;
+ my $test = $3;
+ my $otype = $4;
+ my $trail = $5;
+ my $type = lc($otype);
+ my $op = "!";
+ if (("$test" eq "==" && "$type" eq "true") ||
+ ("$test" eq "!=" && "$type" eq "false")) {
+ $op = "";
+ }
+ WARN("BOOL_COMPARISON",
+ "Using comparison to $otype is poor style. Use '${lead}${op}${arg}${trail}'\n" . $herecurr);
+ }
+
# check for semaphores initialized locked
if ($line =~ /^.\s*sema_init.+,\W?0\W?\)/) {
WARN("CONSIDER_COMPLETION",
On Tue, Apr 09, 2013 at 08:17:14PM -0700, Joe Perches wrote:
> Comparisons of A to true and false are better written
> as A and !A.
>
> Bleat a message on use.
>
> Signed-off-by: Joe Perches <[email protected]>
> ---
> scripts/checkpatch.pl | 17 +++++++++++++++++
> 1 file changed, 17 insertions(+)
>
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index 3fb6d86..080e7f6 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -3538,6 +3538,23 @@ sub process {
> "Using yield() is generally wrong. See yield() kernel-doc (sched/core.c)\n" . $herecurr);
> }
>
> +# check for comparisons against true and false
> + if ($line =~ /\+\s*(.*?)($Lval)\s*(==|\!=)\s*(true|false)\b(.*)$/i) {
> + my $lead = $1;
> + my $arg = $2;
> + my $test = $3;
> + my $otype = $4;
> + my $trail = $5;
> + my $type = lc($otype);
> + my $op = "!";
> + if (("$test" eq "==" && "$type" eq "true") ||
> + ("$test" eq "!=" && "$type" eq "false")) {
> + $op = "";
> + }
> + WARN("BOOL_COMPARISON",
> + "Using comparison to $otype is poor style. Use '${lead}${op}${arg}${trail}'\n" . $herecurr);
In a complex case such as a + b == false will this do the right thing?
Not that I am sure that adding bools makes sense but assuming there is
some valid complex lval.
-apw
On Wed, 2013-04-10 at 10:33 +0100, Andy Whitcroft wrote:
> On Tue, Apr 09, 2013 at 08:17:14PM -0700, Joe Perches wrote:
> > Comparisons of A to true and false are better written
> > as A and !A.
[]
> In a complex case such as a + b == false will this do the right thing?
Very sensible question. No it won't.
checkpatch doesn't understand expressions
very well nor does it understand precedence
operations between expressions and tests.
I did run it against the kernel tree and there
weren't any I noticed like that though.
> Not that I am sure that adding bools makes sense but assuming there is
> some valid complex lval.
$Lval in this case is a single variable and
is neither a function nor an expression.
It doesn't match on:
if (func(x, y) == true)
or
if ((x | y) == true)
because of the ) before the ==
It will falsely match on expressions like:
if (x + y == true)
but as far as I can tell there aren't any
uses like that in the kernel tree.
$ git grep -E "(==|\!=)\s*(true|false)\b" | \
cut -f2- -d":" |grep -P "\b(\+|\-)\b"
nor are there any and/or bit operator.
When I tried adding a test for:
"$Constant == $Lval"
instead of
"$Lval == $Constant"
like
0 == foo
instead of
foo == 0
there were _way_ too many false positives of
the $Expression sort that I didn't add that test.
cheers, Joe
On Wed, Apr 10, 2013 at 04:27:51AM -0700, Joe Perches wrote:
> like
> 0 == foo
> instead of
> foo == 0
>
> there were _way_ too many false positives of
> the $Expression sort that I didn't add that test.
Makes sense then as it is.
Thanks.
-apw
On Tue, 09 Apr 2013 20:17:14 -0700 Joe Perches <[email protected]> wrote:
> Comparisons of A to true and false are better written
> as A and !A.
>
> Bleat a message on use.
hm. I'm counting around 1,100 instances of "== true" and "== false".
That's a lot of people to shout at. Is it really worthwhile?
"foo==true" is a bit of a waste of space but I can't say that I find it
terribly offensive.
On Wed, 2013-04-10 at 15:57 -0700, Andrew Morton wrote:
> On Tue, 09 Apr 2013 20:17:14 -0700 Joe Perches <[email protected]> wrote:
> > Comparisons of A to true and false are better written
> > as A and !A.
> > Bleat a message on use.
> hm. I'm counting around 1,100 instances of "== true" and "== false".
And about all of them in are staging, where I think they
really should be fixed.
$ find . -maxdepth 2 -type d \
while read file ; do \
echo "$(git grep -E '(==|\!=)\s*(true|false)' $file | wc -l) $file"; \
done | sort -rn | head -10
1375 .
1298 ./drivers
1055 ./drivers/staging
63 ./drivers/net
59 ./drivers/gpu
24 ./net
20 ./drivers/media
17 ./net/nfc
13 ./fs
11 ./drivers/usb
> That's a lot of people to shout at.
Not really.
> Is it really worthwhile?
I think so.
On Wed, Apr 10, 2013 at 03:57:51PM -0700, Andrew Morton wrote:
> On Tue, 09 Apr 2013 20:17:14 -0700 Joe Perches <[email protected]> wrote:
>
> > Comparisons of A to true and false are better written
> > as A and !A.
> >
> > Bleat a message on use.
>
> hm. I'm counting around 1,100 instances of "== true" and "== false".
>
> That's a lot of people to shout at. Is it really worthwhile?
> "foo==true" is a bit of a waste of space but I can't say that I find it
> terribly offensive.
It would be interesting to see how many people have historically screwed
up and used (!a) when they mean (a) and vice versa, versus spelling
it out longform. I'd be surprised if the results weren't skewed
in favour of the more verbose form.
Dave
On Wed, 2013-04-10 at 22:14 -0400, Dave Jones wrote:
> It would be interesting to see how many people have historically screwed
> up and used (!a) when they mean (a) and vice versa, versus spelling
> it out longform. I'd be surprised if the results weren't skewed
> in favour of the more verbose form.
I think it'd be interesting too though I'd
take the other side.
On Wed, Apr 10, 2013 at 10:14:15PM -0400, Dave Jones wrote:
> On Wed, Apr 10, 2013 at 03:57:51PM -0700, Andrew Morton wrote:
> > On Tue, 09 Apr 2013 20:17:14 -0700 Joe Perches <[email protected]> wrote:
> >
> > > Comparisons of A to true and false are better written
> > > as A and !A.
> > >
> > > Bleat a message on use.
> >
> > hm. I'm counting around 1,100 instances of "== true" and "== false".
> >
> > That's a lot of people to shout at. Is it really worthwhile?
> > "foo==true" is a bit of a waste of space but I can't say that I find it
> > terribly offensive.
>
> It would be interesting to see how many people have historically screwed
> up and used (!a) when they mean (a) and vice versa, versus spelling
> it out longform. I'd be surprised if the results weren't skewed
> in favour of the more verbose form.
I see a the occasional reversed test in Smatch but normally these
kind of bugs are detected with basic testing so they are rare.
regards,
dan carpenter
On Thu, 2013-04-11 at 11:19 +0300, Dan Carpenter wrote:
> On Wed, Apr 10, 2013 at 10:14:15PM -0400, Dave Jones wrote:
> > It would be interesting to see how many people have historically screwed
> > up and used (!a) when they mean (a) and vice versa, versus spelling
> > it out longform. I'd be surprised if the results weren't skewed
> > in favour of the more verbose form.
>
> I see a the occasional reversed test in Smatch but normally these
> kind of bugs are detected with basic testing so they are rare.
I'd guess the most common error would be using
an int comparison when the value is not 0 or 1.
Non-zero is still "true" but isn't == true.
Dave Jones <[email protected]> writes:
> On Wed, Apr 10, 2013 at 03:57:51PM -0700, Andrew Morton wrote:
> > On Tue, 09 Apr 2013 20:17:14 -0700 Joe Perches <[email protected]> wrote:
> >
> > > Comparisons of A to true and false are better written
> > > as A and !A.
> > >
> > > Bleat a message on use.
> >
> > hm. I'm counting around 1,100 instances of "== true" and "== false".
> >
> > That's a lot of people to shout at. Is it really worthwhile?
> > "foo==true" is a bit of a waste of space but I can't say that I find it
> > terribly offensive.
>
> It would be interesting to see how many people have historically screwed
> up and used (!a) when they mean (a) and vice versa, versus spelling
> it out longform. I'd be surprised if the results weren't skewed
> in favour of the more verbose form.
You have to consider that it is still possible to reverse the operator
even if spelling it out, so you don't really gain anything:
bjorn@nemi:/usr/local/src/git/linux$ git grep -E '!=\s*(true|false)'|wc -l
63
and since most of these compare to true, they are also at risk wrt
integers:
bjorn@nemi:/usr/local/src/git/linux$ git grep -E '!=\s*true'|wc -l
54
Based on a quick look at a few of these I guess they are mostly OK,
testing against bool values. But I felt I had to share this little gem
which showed up in drivers/gpu/drm/exynos/exynos_drm_vidi.c:
static int vidi_power_on(struct vidi_context *ctx, bool enable)
{
struct exynos_drm_subdrv *subdrv = &ctx->subdrv;
struct device *dev = subdrv->dev;
DRM_DEBUG_KMS("%s\n", __FILE__);
if (enable != false && enable != true)
return -EINVAL;
..
That's taking failsafe to the next step :)
Bjørn
On Thu, 2013-04-11 at 13:56 +0200, Bj?rn Mork wrote:
> I felt I had to share this little gem
> which showed up in drivers/gpu/drm/exynos/exynos_drm_vidi.c:
>
> static int vidi_power_on(struct vidi_context *ctx, bool enable)
> {
> struct exynos_drm_subdrv *subdrv = &ctx->subdrv;
> struct device *dev = subdrv->dev;
>
> DRM_DEBUG_KMS("%s\n", __FILE__);
>
> if (enable != false && enable != true)
> return -EINVAL;
> ..
> That's taking failsafe to the next step :)
Cute. It's a relatively new driver too so I'd guess
the "int -> bool conversion leftover" defense isn't
too likely.