Re: [PATCH v4 1/4] staging: rtl8723bs: core: Remove true and false comparison

[Dan Carpenter]

On Sat, Oct 23, 2021 at 01:05:47PM +0530, Kushal Kothari wrote:
>  	struct mlme_priv *pmlmepriv = &(padapter->mlmepriv);
>  	u8 mstatus;
>  
> -	if ((check_fwstate(pmlmepriv, WIFI_ADHOC_MASTER_STATE) == true)
> -		|| (check_fwstate(pmlmepriv, WIFI_ADHOC_STATE) == true)) {
> +	if ((check_fwstate(pmlmepriv, WIFI_ADHOC_MASTER_STATE)) ||
> +		(check_fwstate(pmlmepriv, WIFI_ADHOC_STATE))) {
>  		return;
>  	}
>  

This is a "let it slide" moment.

Reviewed-by: Dan Carpenter <dan.carpenter@xxxxxxxxxx>

Normally would keep my mouth shut but we had already discussed it a bit
and it's really important to know the rules.  In your original patch
you did this correctly, and then a reviewer made a comment about a
different set of parentheses and you modified this one as well so now
it's wrong.  The extra parens get removed in [PATCH 2/2] so, whatever,
it's fine.

The rule is that if you change a line of code you are allowed to make
small changes to fix the style to make checkpatch happy about *THAT
LINE*.  It's not required.  Try to avoid making too many unrelated
changes if it's going to make reviewing difficult.

But I don't want to see three patches fixing the style for a single line
of code.  You can take it too far in either direction.  We had a guy
who was re-writing all the error handling for a function but he would do
it in 5 to 8 patches.  It was crazy hard to review.  He introduced a lot
of bugs as well.  It would have been easier to review as one patch.

But if you change a line and your change introduces a checkpatch warning
then you *must* change the line.  So here, removing the == true, means
you *must* remove the extra parentheses.

But it's fine.  Now you know the rules and can do correctly going
forward.

regards,
dan carpenter