[PATCH 6/9] staging: brcm80211: fix checkpatch error 'assignment in if condition'

Jason jason at lakedaemon.net
Thu Sep 30 01:35:57 UTC 2010


Joe,

On 09/29/2010 06:25 PM, Joe Perches wrote:
> On Wed, 2010-09-29 at 14:19 -0400, jason wrote:
>> Henry Ptasinski wrote:
>>> On Mon, Sep 27, 2010 at 01:37:18PM -0700, Jason Cooper wrote:
>>>> @@ -1847,7 +1858,12 @@ dhd_add_if(dhd_info_t *dhd, int ifidx, void *handle, char *name,
>>>>  	ASSERT(dhd && (ifidx < DHD_MAX_IFS));
>>>>  
>>>>  	ifp = dhd->iflist[ifidx];
>>>> -	if (!ifp && !(ifp = MALLOC(dhd->pub.osh, sizeof(dhd_if_t)))) {
>>>> +	if (!ifp) {
>>>> +		DHD_ERROR(("%s: dhd->iflist[ifidx] null\n", __func__));
>>>> +		return -EINVAL;
>>>> +	}
>>>> +	ifp = MALLOC(dhd->pub.osh, sizeof(dhd_if_t));
>>>> +	if (!ifp) {
>>>>  		DHD_ERROR(("%s: OOM - dhd_if_t\n", __func__));
>>>>  		return -ENOMEM;
>>>>  	}
>>>
>>> I think you changed the logic here from AND to OR.  I believe this would
>>> be more correct:
>>>
>>> 	ifp = MALLOC(dhd->pub.osh, sizeof(dhd_if_t));
>>> 	if (!(dhd->iflist[ifidx]) && (!ifp)) {
>>> 		DHD_ERROR(("%s: OOM - dhd_if_t\n", __func__));
>>> 		return -ENOMEM;
>>> 	}
>>>
>>
>> I was attempting to remove the checkpatch.pl error with as little
>> interpretation as possible.
> 
> Not all checkpatch output needs to be fixed.
> 
> Sometimes the best change is no change at all.
> 

Agreed.

> The current code is straightforward and intelligible.
> The proposed changes make it worse.
> 

Ack.  I'll revert to original when I resubmit.

> You might remove the kmalloc wrappers though.
> 

That is already planned for a separate patch series. ;-)

thx,

Jason.



More information about the devel mailing list