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.
Basically, checkpatch only recognizes positive ints as constants.
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.
Most all of the existing uses seem to be in drivers/scsi/pm8001/pm8001_hwi.c
$ git grep -P 'if\s*\(\s*\-\d+\s*(?:<=|>=|==|<|>)' -- '*.[ch]'
drivers/media/i2c/msp3400-driver.c: if (-1 == scarts[out][in + 1])
drivers/media/pci/bt8xx/bttv-driver.c: if (-1 == formats[i].fourcc)
drivers/media/pci/saa7134/saa7134-tvaudio.c: if (-1 == secondary)
drivers/media/pci/saa7146/mxb.c: if (-1 ==
mxb_saa7740_init[i].length)
drivers/media/usb/s2255/s2255drv.c: if (-1 == formats[i].fourcc)
drivers/net/ieee802154/mrf24j40.c: } else if (-1000 >= mbm && mbm > -2000)
{
drivers/net/ieee802154/mrf24j40.c: } else if (-2000 >= mbm && mbm > -3000)
{
drivers/net/ieee802154/mrf24j40.c: } else if (-3000 >= mbm && mbm > -4000)
{
drivers/net/wireless/broadcom/brcm80211/brcmsmac/phy/phy_lcn.c: if (-1 ==
*gain_index) {
drivers/parisc/eisa_enumerator.c: if (-1==init_slot(i+1, es)) {
drivers/scsi/pm8001/pm8001_hwi.c: if (-1 == pm8001_bar4_shift(pm8001_ha,
drivers/scsi/pm8001/pm8001_hwi.c: if (-1 == pm8001_bar4_shift(pm8001_ha,
drivers/scsi/pm8001/pm8001_hwi.c: if (-1 == pm8001_bar4_shift(pm8001_ha,
drivers/scsi/pm8001/pm8001_hwi.c: if (-1 == pm8001_bar4_shift(pm8001_ha,
drivers/scsi/pm8001/pm8001_hwi.c: if (-1 ==
pm8001_bar4_shift(pm8001_ha, GSM_SM_BASE)) {
drivers/scsi/pm8001/pm8001_hwi.c: if (-1 == check_fw_ready(pm8001_ha)) {
drivers/scsi/pm8001/pm8001_hwi.c: if (-1 ==
pm8001_bar4_shift(pm8001_ha, GSM_SM_BASE)) {
drivers/scsi/pm8001/pm8001_hwi.c: if (-1 ==
pm8001_bar4_shift(pm8001_ha, RB6_ACCESS_REG)) {
drivers/scsi/pm8001/pm8001_hwi.c: if (-1 == pm8001_bar4_shift(pm8001_ha,
MBIC_AAP1_ADDR_BASE)) {
drivers/scsi/pm8001/pm8001_hwi.c: if (-1 == pm8001_bar4_shift(pm8001_ha,
MBIC_IOP_ADDR_BASE)) {
drivers/scsi/pm8001/pm8001_hwi.c: if (-1 == pm8001_bar4_shift(pm8001_ha,
GSM_ADDR_BASE)) {
drivers/scsi/pm8001/pm8001_hwi.c: if (-1 == pm8001_bar4_shift(pm8001_ha,
GPIO_ADDR_BASE)) {
drivers/scsi/pm8001/pm8001_hwi.c: if (-1 == pm8001_bar4_shift(pm8001_ha,
SPC_TOP_LEVEL_ADDR_BASE)) {
drivers/scsi/pm8001/pm8001_hwi.c: if (-1 == pm8001_bar4_shift(pm8001_ha,
GSM_ADDR_BASE)) {
drivers/scsi/pm8001/pm8001_hwi.c: if (-1 == pm8001_bar4_shift(pm8001_ha,
SPC_TOP_LEVEL_ADDR_BASE)) {
drivers/scsi/pm8001/pm8001_hwi.c: if (-1 ==
pm8001_bar4_shift(pm8001_ha,
drivers/scsi/pm8001/pm8001_hwi.c: if (-1 ==
pm80xx_bar4_shift(pm8001_ha,
drivers/scsi/pm8001/pm8001_hwi.c: if (-1 == pm8001_bar4_shift(pm8001_ha,
0))
drivers/scsi/pm8001/pm8001_sas.c: if (-1 ==
pm8001_bar4_shift(pm8001_ha,
drivers/scsi/pm8001/pm80xx_hwi.c: if (-1 == check_fw_ready(pm8001_ha)) {
drivers/scsi/pm8001/pm80xx_hwi.c: if (-1 == check_fw_ready(pm8001_ha)) {
drivers/scsi/scsi_debug.c: if (-1 == res)
drivers/scsi/scsi_debug.c: if (-1 == ret) {
drivers/staging/fbtft/fb_agm1264k-fl.c: if (-1 == coeff) {
fs/btrfs/check-integrity.c: if (-1 == sf->i) {
fs/btrfs/check-integrity.c: if (-1 == sf->i) {
fs/btrfs/check-integrity.c: } else if (-1 ==
btrfsic_check_all_ref_blocks(state,
fs/isofs/util.c: if (-52 <= tz && tz <= 52)
lib/test_bpf.c: "JMP_JSLT_K: Signed jump: if (-2 < -1) return 1",
lib/test_bpf.c: "JMP_JSLT_K: Signed jump: if (-1 < -1) return 0",
lib/test_bpf.c: "JMP_JSGT_K: Signed jump: if (-1 > -2) return 1",
lib/test_bpf.c: "JMP_JSGT_K: Signed jump: if (-1 > -1) return 0",
lib/test_bpf.c: "JMP_JSLE_K: Signed jump: if (-2 <= -1) return 1",
lib/test_bpf.c: "JMP_JSLE_K: Signed jump: if (-1 <= -1) return 1",
lib/test_bpf.c: "JMP_JSGE_K: Signed jump: if (-1 >= -2) return 1",
lib/test_bpf.c: "JMP_JSGE_K: Signed jump: if (-1 >= -1) return 1",
lib/test_bpf.c: "JMP_JGT_K: Unsigned jump: if (-1 > 1) return 1",
lib/test_bpf.c: "JMP_JSGT_X: Signed jump: if (-1 > -2) return 1",
lib/test_bpf.c: "JMP_JSGT_X: Signed jump: if (-1 > -1) return 0",
lib/test_bpf.c: "JMP_JSLT_X: Signed jump: if (-2 < -1) return 1",
lib/test_bpf.c: "JMP_JSLT_X: Signed jump: if (-1 < -1) return 0",
lib/test_bpf.c: "JMP_JSGE_X: Signed jump: if (-1 >= -2) return 1",
lib/test_bpf.c: "JMP_JSGE_X: Signed jump: if (-1 >= -1) return 1",
lib/test_bpf.c: "JMP_JSLE_X: Signed jump: if (-2 <= -1) return 1",
lib/test_bpf.c: "JMP_JSLE_X: Signed jump: if (-1 <= -1) return 1",
lib/test_bpf.c: "JMP_JGT_X: Unsigned jump: if (-1 > 1) return 1",
sound/pci/rme9652/hdspm.c: if (-1 == val)
sound/usb/usx2y/usbusx2y.c: if (-2 == us428ctls->CtlSnapShotLast) {
tools/testing/selftests/net/mptcp/mptcp_connect.c: if (-1 ==
setsockopt(sock, SOL_SOCKET, SO_REUSEADDR, &one,
