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
