checkpatch.pl false positive? "ERROR: do not use assignment in if condition"

checkpatch.pl false positive? "ERROR: do not use assignment in if condition"

[Tilman Schmidt]

[-- Attachment #1: Type: text/plain, Size: 1106 bytes --]

The command
./scripts/checkpatch.pl -f drivers/isdn/gigaset/bas-gigaset.c 
produces a lot of "ERROR" messages like these:

ERROR: do not use assignment in if condition
#608: FILE: isdn/gigaset/bas-gigaset.c:608:
+       if ((ret = usb_submit_urb(ucs->urb_cmd_in, GFP_ATOMIC)) != 0) {

ERROR: do not use assignment in if condition
#745: FILE: isdn/gigaset/bas-gigaset.c:745:
+               if ((ucs->rcvbuf = kmalloc(l, GFP_ATOMIC)) == NULL) {

ERROR: do not use assignment in if condition
#753: FILE: isdn/gigaset/bas-gigaset.c:753:
+               if ((rc = atread_submit(cs, BAS_TIMEOUT)) < 0) {

As far as I can see there's nothing wrong with these lines. In particular,
I cannot find anything in Documentation/CodingStyle that would prohibit an
assignment inside an 'if' condition.

PS: I know the file has other problems. That's why I'm trying to
checkpatch it in the first place.

-- 
Tilman Schmidt                    E-Mail: tilman@imap.cc
Bonn, Germany
Diese Nachricht besteht zu 100% aus wiederverwerteten Bits.
Ungeöffnet mindestens haltbar bis: (siehe Rückseite)


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 254 bytes --]

Re: checkpatch.pl false positive? "ERROR: do not use assignment in if condition"

[Randy Dunlap]

On Tue, 20 Oct 2009 17:50:45 +0200 Tilman Schmidt wrote:

> The command
> ./scripts/checkpatch.pl -f drivers/isdn/gigaset/bas-gigaset.c 
> produces a lot of "ERROR" messages like these:
> 
> ERROR: do not use assignment in if condition
> #608: FILE: isdn/gigaset/bas-gigaset.c:608:
> +       if ((ret = usb_submit_urb(ucs->urb_cmd_in, GFP_ATOMIC)) != 0) {
> 
> ERROR: do not use assignment in if condition
> #745: FILE: isdn/gigaset/bas-gigaset.c:745:
> +               if ((ucs->rcvbuf = kmalloc(l, GFP_ATOMIC)) == NULL) {
> 
> ERROR: do not use assignment in if condition
> #753: FILE: isdn/gigaset/bas-gigaset.c:753:
> +               if ((rc = atread_submit(cs, BAS_TIMEOUT)) < 0) {
> 
> As far as I can see there's nothing wrong with these lines. In particular,
> I cannot find anything in Documentation/CodingStyle that would prohibit an
> assignment inside an 'if' condition.

Yes, we don't try to list Every Possible Problem in CodingStyle,
but emails on lkml over the past several years try to discourage such
assignments inside if's because they can be confusing, difficult to read,
and generally are not helping to produce any better code output than
using:
	ret = usb_submit_urb(args);
	if (ret) {
		foo;
		bar;
		blah();
	}

> PS: I know the file has other problems. That's why I'm trying to
> checkpatch it in the first place.



---
~Randy

Re: checkpatch.pl false positive? "ERROR: do not use assignment in if condition"

[Andy Whitcroft]

On Tue, Oct 20, 2009 at 08:58:20AM -0700, Randy Dunlap wrote:
> On Tue, 20 Oct 2009 17:50:45 +0200 Tilman Schmidt wrote:
> 
> > The command
> > ./scripts/checkpatch.pl -f drivers/isdn/gigaset/bas-gigaset.c 
> > produces a lot of "ERROR" messages like these:
> > 
> > ERROR: do not use assignment in if condition
> > #608: FILE: isdn/gigaset/bas-gigaset.c:608:
> > +       if ((ret = usb_submit_urb(ucs->urb_cmd_in, GFP_ATOMIC)) != 0) {
> > 
> > ERROR: do not use assignment in if condition
> > #745: FILE: isdn/gigaset/bas-gigaset.c:745:
> > +               if ((ucs->rcvbuf = kmalloc(l, GFP_ATOMIC)) == NULL) {
> > 
> > ERROR: do not use assignment in if condition
> > #753: FILE: isdn/gigaset/bas-gigaset.c:753:
> > +               if ((rc = atread_submit(cs, BAS_TIMEOUT)) < 0) {
> > 
> > As far as I can see there's nothing wrong with these lines. In particular,
> > I cannot find anything in Documentation/CodingStyle that would prohibit an
> > assignment inside an 'if' condition.
> 
> Yes, we don't try to list Every Possible Problem in CodingStyle,
> but emails on lkml over the past several years try to discourage such
> assignments inside if's because they can be confusing, difficult to read,
> and generally are not helping to produce any better code output than
> using:
> 	ret = usb_submit_urb(args);
> 	if (ret) {
> 		foo;
> 		bar;
> 		blah();
> 	}
> 
> > PS: I know the file has other problems. That's why I'm trying to
> > checkpatch it in the first place.

What he said :).  We try and codify the preferred style where there is
a preference.  That preference was made by reviewers as it is much
harder to accidentally do the following when you meant the assignment or
visa versa:

    if ((rc == foo) < 10)

-apw

Re: checkpatch.pl false positive? "ERROR: do not use assignment in if condition"

[Tilman Schmidt]

[-- Attachment #1: Type: text/plain, Size: 3165 bytes --]

Andy Whitcroft schrieb:
> On Tue, Oct 20, 2009 at 08:58:20AM -0700, Randy Dunlap wrote:
>> On Tue, 20 Oct 2009 17:50:45 +0200 Tilman Schmidt wrote:
>>
>>> The command
>>> ./scripts/checkpatch.pl -f drivers/isdn/gigaset/bas-gigaset.c 
>>> produces a lot of "ERROR" messages like these:
>>>
>>> ERROR: do not use assignment in if condition
>>> #608: FILE: isdn/gigaset/bas-gigaset.c:608:
>>> +       if ((ret = usb_submit_urb(ucs->urb_cmd_in, GFP_ATOMIC)) != 0) {
>>>
>>> ERROR: do not use assignment in if condition
>>> #745: FILE: isdn/gigaset/bas-gigaset.c:745:
>>> +               if ((ucs->rcvbuf = kmalloc(l, GFP_ATOMIC)) == NULL) {
>>>
>>> ERROR: do not use assignment in if condition
>>> #753: FILE: isdn/gigaset/bas-gigaset.c:753:
>>> +               if ((rc = atread_submit(cs, BAS_TIMEOUT)) < 0) {
>>>
>>> As far as I can see there's nothing wrong with these lines. In particular,
>>> I cannot find anything in Documentation/CodingStyle that would prohibit an
>>> assignment inside an 'if' condition.
>> Yes, we don't try to list Every Possible Problem in CodingStyle,

Well, that's not very nice towards small time developers like me.
I try to follow CodingStyle as best I can, only to get smacked with
a checkpatch ERROR for something that isn't even hinted at there.
If a way of coding is such an abomination that it merits an ERROR
from checkpatch (meaning " change this or see your submission
rejected"), then it also merits being mentioned in CodingStyle.

>> but emails on lkml over the past several years try to discourage such
>> assignments inside if's because they can be confusing, difficult to read,
>> and generally are not helping to produce any better code output than
>> using:
>> 	ret = usb_submit_urb(args);
>> 	if (ret) {
>> 		foo;
>> 		bar;
>> 		blah();
>> 	}

I don't agree, but judging from your wording you don't want to hear any
arguments, so I'll leave it at that. I just want to know *beforehand*
what I have to do in order to get my code merged.

> What he said :).  We try and codify the preferred style where there is
> a preference.  That preference was made by reviewers as it is much
> harder to accidentally do the following when you meant the assignment or
> visa versa:

But does a mere preference by some reviewers warrant an ERROR message,
when even a line that exceeds the 80 character limit, which is
elaborately justified in CodingStyle, produces only a warning?

Again, I don't want to start a discussion about that preference. I know
now that it exists, and I am in no position to question it. But how many
of those preferences may yet turn up to bite me? If people are feeling
so strongly about a preference that those who do not honor it must be
slapped with a checkpatch ERROR then they should also find the energy
to add an appropriate warning about that to CodingStyle so that coders
have a chance to avoid it. That's only fair.

Thanks,
Tilman

-- 
Tilman Schmidt                    E-Mail: tilman@imap.cc
Bonn, Germany
Diese Nachricht besteht zu 100% aus wiederverwerteten Bits.
Ungeöffnet mindestens haltbar bis: (siehe Rückseite)


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 254 bytes --]