Re: [PATCH]: if (cond); foo() in firewire

on 22/06/2009 07:54 Steve Kargl said the following:
> On Mon, Jun 22, 2009 at 12:04:49PM +0800, Adrian Chadd wrote:
>> 2009/6/21 Roman Divacky <rdivacky_at_freebsd.org>:
>>> hi
>>>
>>> is this patch correct? may I commit it?
>>>
>>> Index: ../../../dev/firewire/fwdev.c
>>> ===================================================================
>>> --- ../../../dev/firewire/fwdev.c       (revision 194573)
>>> +++ ../../../dev/firewire/fwdev.c       (working copy)
>>> _at__at_ -443,7 +443,7 _at__at_
>>>        xfer->send.pay_len = uio->uio_resid;
>>>        if (uio->uio_resid > 0) {
>>>                if ((err = uiomove((caddr_t)&xfer->send.payload[0],
>>> -                   uio->uio_resid, uio)));
>>> +                   uio->uio_resid, uio)))
>>>                        goto out;
>>>        }
>>>
>>>
>>> another bug found by the "useless warnings in clang" ;)
>> Is clang also evaluating all subsequent execution paths to tell you
>> what the change in program flow is? :)
>>
>> I hate to be the harbinger of evilness, but I'd at least attempt a
>> cursory glance at the code to make sure subsequent code is doing the
>> right thing. (It certainly looks like a vanilla userland transfer!)

You confuse me. It is a "vanilla userland transfer", but so?
Current code always goes to "out" label regardless if uimove succeeded or not.
I think the idea was to go "out" only if uimove failed and execute some code
between if and out-label otherwise.


> I agree with you.  Nothing like side effects to screw up 
> a persons clang.


You confuse me, what this has to do with side-effects?
I think that Clang is right - 'if' without "then" is suspicious because either you
have a useless/redundant 'if' statement (as in you example below - just call
side_effect(&i) without putting it under if) or you accidentally put semicolon
where you shouldn't have.

> #include <stdio.h>
> #include <stdlib.h>
> 
> static int
> side_effect(int *i)
> {
>    *i = 42;
>    return 0;
> }
> 
> int 
> main(void)
> {
>    int i;
>    if (side_effect(&i));
>    if (i == 42)
>       printf("%d\n", i);
>    return 0;
> }
> 


-- 
Andriy Gapon