Re: [PATCH] checkpatch: Warn on comparisons to true and false
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 = >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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] checkpatch: Warn on comparisons to true and false
Dave Jones 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 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 = >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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] checkpatch: Warn on comparisons to true and false
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. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] checkpatch: Warn on comparisons to true and false
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 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] checkpatch: Warn on comparisons to true and false
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 j...@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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] checkpatch: Warn on comparisons to true and false
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. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] checkpatch: Warn on comparisons to true and false
Dave Jones da...@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 j...@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
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] checkpatch: Warn on comparisons to true and false
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.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] checkpatch: Warn on comparisons to true and false
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. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] checkpatch: Warn on comparisons to true and false
On Wed, Apr 10, 2013 at 03:57:51PM -0700, Andrew Morton wrote: > On Tue, 09 Apr 2013 20:17:14 -0700 Joe Perches 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] checkpatch: Warn on comparisons to true and false
On Wed, 2013-04-10 at 15:57 -0700, Andrew Morton wrote: > On Tue, 09 Apr 2013 20:17:14 -0700 Joe Perches 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. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] checkpatch: Warn on comparisons to true and false
On Tue, 09 Apr 2013 20:17:14 -0700 Joe Perches 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. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] checkpatch: Warn on comparisons to true and false
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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] checkpatch: Warn on comparisons to true and false
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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] checkpatch: Warn on comparisons to true and false
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
> ---
> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] checkpatch: Warn on comparisons to true and false
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 j...@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
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] checkpatch: Warn on comparisons to true and false
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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] checkpatch: Warn on comparisons to true and false
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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] checkpatch: Warn on comparisons to true and false
On Tue, 09 Apr 2013 20:17:14 -0700 Joe Perches j...@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. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] checkpatch: Warn on comparisons to true and false
On Wed, 2013-04-10 at 15:57 -0700, Andrew Morton wrote: On Tue, 09 Apr 2013 20:17:14 -0700 Joe Perches j...@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. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] checkpatch: Warn on comparisons to true and false
On Wed, Apr 10, 2013 at 03:57:51PM -0700, Andrew Morton wrote: On Tue, 09 Apr 2013 20:17:14 -0700 Joe Perches j...@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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] checkpatch: Warn on comparisons to true and false
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. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
