Re: [PATCH 10/11] Staging: vt6656: Combined nested conditions

[Dan Carpenter]

On Thu, Dec 26, 2013 at 07:55:39PM +0100, Sebastian Rachuj wrote:
> -	if (pbyDesireSSID != NULL) {
> -		if (((PWLAN_IE_SSID) pbyDesireSSID)->len != 0)
> -			pSSID = (PWLAN_IE_SSID) pbyDesireSSID;
> -	}
> +	if ((pbyDesireSSID != NULL) &&
> +	    (((PWLAN_IE_SSID) pbyDesireSSID)->len != 0))
> +		pSSID = (PWLAN_IE_SSID) pbyDesireSSID;

That's a lot of extra parentheses!

This patch is fine, but for future reference, adding a double negative
" != NULL" almost never doesn't makes the code less readable.  If we
remove it then this fits on one line.

	if (pbyDesireSSID && ((PWLAN_IE_SSID) pbyDesireSSID)->len != 0)
		pSSID = (PWLAN_IE_SSID) pbyDesireSSID;

The casting is horrible and this is garbage staging code.  If the types
were cleaned up then it would look like:

	if (pbyDesireSSID && pbyDesireSSID->len != 0)
		pSSID = pbyDesireSSID;

The naming is also totally bogus, of course.  What does "pby" mean?  I
think "Desire" is clear but I don't think it's native English.  The "p"
in "pSSID" probably stands for pointer???  It's as if someone
deliberately made every single character of this code as terrible as can
be.

	if (new_ssid && new_ssid->len != 0)
		ssid = new_ssid;

Technically " != 0" is the same as " != NULL" but " != 0" tells a story
and makes the code more readable.


> +				} else if ((pMgmt->eConfigMode == WMAC_CONFIG_AUTO) ||
> +					((pMgmt->eConfigMode == WMAC_CONFIG_IBSS_STA) && WLAN_GET_CAP_INFO_IBSS(pCurrBSS->wCapInfo)) ||
> +					((pMgmt->eConfigMode == WMAC_CONFIG_ESS_STA) && WLAN_GET_CAP_INFO_ESS(pCurrBSS->wCapInfo))) {

Adding all these extra parentheses makes it harder to tell which are
needed and which are noise.

				} else if (pMgmt->eConfigMode == WMAC_CONFIG_AUTO ||
					   (pMgmt->eConfigMode == WMAC_CONFIG_IBSS_STA &&
					    WLAN_GET_CAP_INFO_IBSS(pCurrBSS->wCapInfo)) ||
					   (pMgmt->eConfigMode == WMAC_CONFIG_ESS_STA &&
					    WLAN_GET_CAP_INFO_ESS(pCurrBSS->wCapInfo)) {

In this case, the extra parentheses were there in the original code but
there are other times where this patch adds a number of pretty bogus
parens.

regards,
dan carpenter

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/