On 3/28/21 3:32 AM, Joe Perches wrote:
> On Sat, 2021-03-27 at 15:26 -0700, Randy Dunlap wrote:
>> On 3/27/21 3:12 PM, Mihai Moldovan wrote:
>>> * On 3/27/21 4:58 PM, Randy Dunlap wrote:
>>>> On 3/27/21 5:01 AM, Mihai Moldovan wrote:
>>>>> + if ((-1 == index) && (index == match_start))
>>>>
>>>> checkpatch doesn't complain about this (and I wonder how it's missed), but
>>>> kernel style is (mostly) "constant goes on right hand side of comparison",
>>>> so
>>>> if ((index == -1) &&
>>>
>>> I can naturally send a V2 with that swapped.
>>>
>>> To my rationale: I made sure to use checkpatch, saw that it was accepted and
>>> even went for a quick git grep -- '-1 ==', which likewise returned enough
>>> results for me to call this consistent with the current code style.
>>>
>>> Maybe those matches were just frowned-upon, but forgotten-to-be-critized
>>> examples of this pattern being used.
>>
>> There is a test for it in checkpatch.pl but I also used checkpatch.pl
>> without it complaining, so I don't know what it takes to make the script
>> complain.
>>
>> if ($lead !~ /(?:$Operators|\.)\s*$/ &&
>> $to !~ /^(?:Constant|[A-Z_][A-Z0-9_]*)$/ &&
>> WARN("CONSTANT_COMPARISON",
>> "Comparisons should place the constant on the
>> right side of the test\n" . $herecurr) &&
>
> Negative values aren't parsed well by the silly script as checkpatch
> isn't a real parser.
Yes, I get that.
> Basically, checkpatch only recognizes positive ints as constants.
OK, good to know.
> So it doesn't recognize uses like:
>
> a * -5 > b
>
> It doesn't parse -5 as a negative constant.
>
> Though here it does seem the line with
> $to !~ /^(?:Constant|[A-Z_][A-Z0-9_]*)$/ &&
> should be
> $to !~ /^(?:$Constant|[A-Z_][A-Z0-9_]*)$/ &&
>
> You are welcome to try to improve checkpatch, but it seems non-trivial
> and a relatively uncommon use in the kernel, so I won't.
I get that too.
> Most all of the existing uses seem to be in drivers/scsi/pm8001/pm8001_hwi.c
[snip]
thanks.
--
~Randy
