2022-04-22 20:32:17

by Jaehee Park

[permalink] [raw]
Subject: [PATCH v4 1/7] staging: r8188eu: remove unnecessary braces in single statement block

Remove braces for single statement block to minimize the number of
empty lines, without loss of readability. Issue found with checkpatch.
WARNING: braces {} are not necessary for single statement blocks

Signed-off-by: Jaehee Park <[email protected]>
---
drivers/staging/r8188eu/core/rtw_mlme.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/staging/r8188eu/core/rtw_mlme.c b/drivers/staging/r8188eu/core/rtw_mlme.c
index 3e9882f89f76..d3f4d7cdfa08 100644
--- a/drivers/staging/r8188eu/core/rtw_mlme.c
+++ b/drivers/staging/r8188eu/core/rtw_mlme.c
@@ -112,9 +112,8 @@ void _rtw_free_mlme_priv(struct mlme_priv *pmlmepriv)

rtw_free_mlme_priv_ie_data(pmlmepriv);

- if (pmlmepriv) {
+ if (pmlmepriv)
vfree(pmlmepriv->free_bss_buf);
- }

}

--
2.25.1


2022-04-22 21:04:55

by Fabio M. De Francesco

[permalink] [raw]
Subject: Re: [PATCH v4 1/7] staging: r8188eu: remove unnecessary braces in single statement block

On giovedì 21 aprile 2022 21:20:26 CEST Jaehee Park wrote:
> Remove braces for single statement block to minimize the number of
> empty lines, without loss of readability. Issue found with checkpatch.
> WARNING: braces {} are not necessary for single statement blocks
>
> Signed-off-by: Jaehee Park <[email protected]>
> ---
> drivers/staging/r8188eu/core/rtw_mlme.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/drivers/staging/r8188eu/core/rtw_mlme.c b/drivers/staging/
r8188eu/core/rtw_mlme.c
> index 3e9882f89f76..d3f4d7cdfa08 100644
> --- a/drivers/staging/r8188eu/core/rtw_mlme.c
> +++ b/drivers/staging/r8188eu/core/rtw_mlme.c
> @@ -112,9 +112,8 @@ void _rtw_free_mlme_priv(struct mlme_priv *pmlmepriv)
>
> rtw_free_mlme_priv_ie_data(pmlmepriv);
>
> - if (pmlmepriv) {
> + if (pmlmepriv)
> vfree(pmlmepriv->free_bss_buf);
> - }
>
> }
>
> --
> 2.25.1
>

Hi Jaehee,

If you just want to remove the braces, your patch is good.

But... Did you ask yourself why this code needs to test if "pmlmepriv" is
valid before calling vfree() on it? Actually, the code doesn't need this
test, it just needs to call "vfree(pmlmepriv)" unconditionally.

OK, I know that this is not directly related to your patch or any other
that is explicitly made just for removing unnecessary braces...

vfree() behaves as a no-op function on NULL pointers, therefore there is no
need to test whether or not "pmlmepriv" is valid.

Thanks,

Fabio M. De Francesco



2022-04-22 21:09:11

by Fabio M. De Francesco

[permalink] [raw]
Subject: Re: [PATCH v4 1/7] staging: r8188eu: remove unnecessary braces in single statement block

On giovedì 21 aprile 2022 21:52:14 CEST Fabio M. De Francesco wrote:
> On giovedì 21 aprile 2022 21:20:26 CEST Jaehee Park wrote:
> > Remove braces for single statement block to minimize the number of
> > empty lines, without loss of readability. Issue found with checkpatch.
> > WARNING: braces {} are not necessary for single statement blocks
> >
> > Signed-off-by: Jaehee Park <[email protected]>
> > ---
> > drivers/staging/r8188eu/core/rtw_mlme.c | 3 +--
> > 1 file changed, 1 insertion(+), 2 deletions(-)
> >
> > diff --git a/drivers/staging/r8188eu/core/rtw_mlme.c b/drivers/staging/
> r8188eu/core/rtw_mlme.c
> > index 3e9882f89f76..d3f4d7cdfa08 100644
> > --- a/drivers/staging/r8188eu/core/rtw_mlme.c
> > +++ b/drivers/staging/r8188eu/core/rtw_mlme.c
> > @@ -112,9 +112,8 @@ void _rtw_free_mlme_priv(struct mlme_priv
*pmlmepriv)
> >
> > rtw_free_mlme_priv_ie_data(pmlmepriv);
> >
> > - if (pmlmepriv) {
> > + if (pmlmepriv)
> > vfree(pmlmepriv->free_bss_buf);
> > - }
> >
> > }
> >
> > --
> > 2.25.1
> >
>
> Hi Jaehee,
>
> If you just want to remove the braces, your patch is good.
>
> But... Did you ask yourself why this code needs to test if "pmlmepriv" is
> valid before calling vfree() on it? Actually, the code doesn't need this
> test, it just needs to call "vfree(pmlmepriv)" unconditionally.

When copy-pasting I overlooked a couple of things... However the argument
is the same: the code doesn't need that 'if' statement before vfree().

I wanted to write "[] it just needs to call "vfree(pmlmepriv-
>free_bss_buf)" unconditionally".

I apologize for not reading twice before sending my email too soon after
writing and moving sentences around.

> OK, I know that this is not directly related to your patch or any other
> that is explicitly made just for removing unnecessary braces...

"pmlmepriv" is valid because we know it from the previous call to
rtw_free_mlme_priv_ie_data(). Therefore the code can dereference
"pmlmepriv".

Furthermore, even if "free_bss_buf" were NULL it wouldn't matter because
"vfree(pmlmepriv->free_bss_buf)" would be a no-op.

Sorry if this inaccurate wording might have confused you about what I was
trying to say.

Thanks,

Fabio