linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] checkpatch: Warn on comparisons to true and false
@ 2013-04-10  3:17 Joe Perches
  2013-04-10  9:33 ` Andy Whitcroft
  2013-04-10 22:57 ` Andrew Morton
  0 siblings, 2 replies; 12+ messages in thread
From: Joe Perches @ 2013-04-10  3:17 UTC (permalink / raw)
  To: Andrew Morton; +Cc: Andy Whitcroft, LKML, Jacob Pan

Comparisons of A to true and false are better written
as A and !A.

Bleat a message on use.

Signed-off-by: Joe Perches <joe@perches.com>
---
 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",



^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] checkpatch: Warn on comparisons to true and false
  2013-04-10  3:17 [PATCH] checkpatch: Warn on comparisons to true and false Joe Perches
@ 2013-04-10  9:33 ` Andy Whitcroft
  2013-04-10 11:27   ` Joe Perches
  2013-04-10 22:57 ` Andrew Morton
  1 sibling, 1 reply; 12+ messages in thread
From: Andy Whitcroft @ 2013-04-10  9:33 UTC (permalink / raw)
  To: Joe Perches; +Cc: Andrew Morton, LKML, Jacob Pan

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 <joe@perches.com>
> ---
>  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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] checkpatch: Warn on comparisons to true and false
  2013-04-10  9:33 ` Andy Whitcroft
@ 2013-04-10 11:27   ` Joe Perches
  2013-04-10 12:41     ` Andy Whitcroft
  0 siblings, 1 reply; 12+ messages in thread
From: Joe Perches @ 2013-04-10 11:27 UTC (permalink / raw)
  To: Andy Whitcroft; +Cc: Andrew Morton, LKML, Jacob Pan

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


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] checkpatch: Warn on comparisons to true and false
  2013-04-10 11:27   ` Joe Perches
@ 2013-04-10 12:41     ` Andy Whitcroft
  0 siblings, 0 replies; 12+ messages in thread
From: Andy Whitcroft @ 2013-04-10 12:41 UTC (permalink / raw)
  To: Joe Perches; +Cc: Andrew Morton, LKML, Jacob Pan

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] checkpatch: Warn on comparisons to true and false
  2013-04-10  3:17 [PATCH] checkpatch: Warn on comparisons to true and false Joe Perches
  2013-04-10  9:33 ` Andy Whitcroft
@ 2013-04-10 22:57 ` Andrew Morton
  2013-04-11  1:07   ` Joe Perches
  2013-04-11  2:14   ` Dave Jones
  1 sibling, 2 replies; 12+ messages in thread
From: Andrew Morton @ 2013-04-10 22:57 UTC (permalink / raw)
  To: Joe Perches; +Cc: Andy Whitcroft, LKML, Jacob Pan

On Tue, 09 Apr 2013 20:17:14 -0700 Joe Perches <joe@perches.com> 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.


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] checkpatch: Warn on comparisons to true and false
  2013-04-10 22:57 ` Andrew Morton
@ 2013-04-11  1:07   ` Joe Perches
  2013-04-11  2:14   ` Dave Jones
  1 sibling, 0 replies; 12+ messages in thread
From: Joe Perches @ 2013-04-11  1:07 UTC (permalink / raw)
  To: Andrew Morton; +Cc: Andy Whitcroft, LKML, Jacob Pan

On Wed, 2013-04-10 at 15:57 -0700, Andrew Morton wrote:
> On Tue, 09 Apr 2013 20:17:14 -0700 Joe Perches <joe@perches.com> 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.



^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] checkpatch: Warn on comparisons to true and false
  2013-04-10 22:57 ` Andrew Morton
  2013-04-11  1:07   ` Joe Perches
@ 2013-04-11  2:14   ` Dave Jones
  2013-04-11  3:47     ` Joe Perches
                       ` (2 more replies)
  1 sibling, 3 replies; 12+ messages in thread
From: Dave Jones @ 2013-04-11  2:14 UTC (permalink / raw)
  To: Andrew Morton; +Cc: Joe Perches, Andy Whitcroft, LKML, Jacob Pan

On Wed, Apr 10, 2013 at 03:57:51PM -0700, Andrew Morton wrote:
 > On Tue, 09 Apr 2013 20:17:14 -0700 Joe Perches <joe@perches.com> 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


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] checkpatch: Warn on comparisons to true and false
  2013-04-11  2:14   ` Dave Jones
@ 2013-04-11  3:47     ` Joe Perches
  2013-04-11  8:19     ` Dan Carpenter
  2013-04-11 11:56     ` Bjørn Mork
  2 siblings, 0 replies; 12+ messages in thread
From: Joe Perches @ 2013-04-11  3:47 UTC (permalink / raw)
  To: Dave Jones; +Cc: Andrew Morton, Andy Whitcroft, LKML, Jacob Pan

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.


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] checkpatch: Warn on comparisons to true and false
  2013-04-11  2:14   ` Dave Jones
  2013-04-11  3:47     ` Joe Perches
@ 2013-04-11  8:19     ` Dan Carpenter
  2013-04-11  8:29       ` Joe Perches
  2013-04-11 11:56     ` Bjørn Mork
  2 siblings, 1 reply; 12+ messages in thread
From: Dan Carpenter @ 2013-04-11  8:19 UTC (permalink / raw)
  To: Dave Jones, Andrew Morton, Joe Perches, Andy Whitcroft, LKML, Jacob Pan

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 <joe@perches.com> 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


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] checkpatch: Warn on comparisons to true and false
  2013-04-11  8:19     ` Dan Carpenter
@ 2013-04-11  8:29       ` Joe Perches
  0 siblings, 0 replies; 12+ messages in thread
From: Joe Perches @ 2013-04-11  8:29 UTC (permalink / raw)
  To: Dan Carpenter; +Cc: Dave Jones, Andrew Morton, Andy Whitcroft, LKML, Jacob Pan

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.



^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] checkpatch: Warn on comparisons to true and false
  2013-04-11  2:14   ` Dave Jones
  2013-04-11  3:47     ` Joe Perches
  2013-04-11  8:19     ` Dan Carpenter
@ 2013-04-11 11:56     ` Bjørn Mork
  2013-04-11 14:25       ` Joe Perches
  2 siblings, 1 reply; 12+ messages in thread
From: Bjørn Mork @ 2013-04-11 11:56 UTC (permalink / raw)
  To: Dave Jones; +Cc: Andrew Morton, Joe Perches, Andy Whitcroft, LKML, Jacob Pan

Dave Jones <davej@redhat.com> 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 <joe@perches.com> 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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] checkpatch: Warn on comparisons to true and false
  2013-04-11 11:56     ` Bjørn Mork
@ 2013-04-11 14:25       ` Joe Perches
  0 siblings, 0 replies; 12+ messages in thread
From: Joe Perches @ 2013-04-11 14:25 UTC (permalink / raw)
  To: Bjørn Mork
  Cc: Dave Jones, Andrew Morton, Andy Whitcroft, LKML, Jacob Pan

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.


^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2013-04-11 14:25 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-04-10  3:17 [PATCH] checkpatch: Warn on comparisons to true and false Joe Perches
2013-04-10  9:33 ` Andy Whitcroft
2013-04-10 11:27   ` Joe Perches
2013-04-10 12:41     ` Andy Whitcroft
2013-04-10 22:57 ` Andrew Morton
2013-04-11  1:07   ` Joe Perches
2013-04-11  2:14   ` Dave Jones
2013-04-11  3:47     ` Joe Perches
2013-04-11  8:19     ` Dan Carpenter
2013-04-11  8:29       ` Joe Perches
2013-04-11 11:56     ` Bjørn Mork
2013-04-11 14:25       ` Joe Perches

This is a public inbox, see mirroring instructions
on how to clone and mirror all data and code used for this inbox