2019-10-11 01:54:20

by Sergey Senozhatsky

Subject: checkpatch: comparisons with a constant on the left

Hi Joe,

I noticed that this code

#if LINUX_VERSION_CODE >= KERNEL_VERSION(4, 18, 0)

triggers checkpatch's warning:

"WARNING: Comparisons should place the constant on
the right side of the test"

Both LINUX_VERSION_CODE and KERNEL_VERSION are constants, so
I'm wondering if it's worth it to improve that check a tiny
bit.

E.g. something utterly trivial:

---
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index cf7543a9d1b2..8a1964c3d9a5 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -4855,6 +4855,7 @@ sub process {
my $to = $4;
my $newcomp = $comp;
if ($lead !~ /(?:$Operators|\.)\s*$/ &&
+ $to !~ /^KERNEL_VERSION.*/ &&
$to !~ /^(?:Constant|[A-Z_][A-Z0-9_]*)$/ &&
WARN("CONSTANT_COMPARISON",
"Comparisons should place the constant on the right side of the test\n" . $herecurr) &&
---

I'm sure you'll have a better idea.

-ss

2019-10-11 03:24:46

by Joe Perches

Subject: Re: checkpatch: comparisons with a constant on the left

On Fri, 2019-10-11 at 10:52 +0900, Sergey Senozhatsky wrote:
> Hi Joe,

Hi Sergey.

> I noticed that this code
>
> #if LINUX_VERSION_CODE >= KERNEL_VERSION(4, 18, 0)
>
> triggers checkpatch's warning:
>
> "WARNING: Comparisons should place the constant on
> the right side of the test"
>
> Both LINUX_VERSION_CODE and KERNEL_VERSION are constants, so
> I'm wondering if it's worth it to improve that check a tiny
> bit.

Probably not.

My preference is for people to ignore checkpatch
message bleats when they don't make overall sense.

checkpatch thinks anything that uses a form like
"name(<args...>)" is a function.

> I'm sure you'll have a better idea.

I suggest reversing the test if it really bothers you.

# if KERNEL_VERSION(4.18.0) < LINUX_VERSION_CODE

but then again just using LINUX_VERSION_CODE emits a
warning message, so it's better to remove whatever is
in the block anyway... <smile>

cheers, Joe

2019-10-20 07:32:49

by Sergey Senozhatsky

Subject: Re: checkpatch: comparisons with a constant on the left

On (10/10/19 20:23), Joe Perches wrote:
> On Fri, 2019-10-11 at 10:52 +0900, Sergey Senozhatsky wrote:
> > Hi Joe,
>
> Hi Sergey.

Hi Joe,

For some reason your reply triggered gmail spam filter; took
me a while to notice and "recover" it from spam folder.

[..]
> > Both LINUX_VERSION_CODE and KERNEL_VERSION are constants, so
> > I'm wondering if it's worth it to improve that check a tiny
> > bit.
>
> Probably not.
>
> My preference is for people to ignore checkpatch
> message bleats when they don't make overall sense.
>
> checkpatch thinks anything that uses a form like
> "name(<args...>)" is a function.

For example, DMA_BIT_MASK(xxx) can look like a function
call yet still be a compile time constant.

Another example could be ARRAY_SIZE(xxx), I guess.

[..]
> but then again just using LINUX_VERSION_CODE emits a
> warning message, so it's better to remove whatever is
> in the block anyway... <smile>

That's certainly right. LINUX_VERSION_CODE should not be in the
code, I agree. I was thinking more about 'const vs const' comparison
in general, less about particular LINUX_VERSION_CODE case.

-ss