checkpatch: comparisons with a constant on the left

checkpatch: comparisons with a constant on the left

[Sergey Senozhatsky]

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

Re: checkpatch: comparisons with a constant on the left

[Joe Perches]

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

Re: checkpatch: comparisons with a constant on the left

[Sergey Senozhatsky]

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