Re: [PATCH v2] staging: rtl8723bs: core: Refactor nested if-else

From: Hans de Goede
Date: Tue Oct 26 2021 - 11:15:21 EST


Hi,

On 10/26/21 17:12, kushal kothari wrote:
> Yes!
> Also initializing  variable pstat->has_legacy_ac = false;  //false
> which is previously set to true before the loop so it can be removed too.

Note please do not make too many changes in a single patch.

It is best to do refactoring like this in small steps making
only 1 change at a time, that makes things a lot easier to
review / to verify that there are no functional changes.

Regards,

Hans


>
> On Tue, Oct 26, 2021 at 7:49 PM Mike Rapoport <mike.rapoport@xxxxxxxxx <mailto:mike.rapoport@xxxxxxxxx>> wrote:
>
> On Tue, Oct 26, 2021 at 07:12:53PM +0530, Kushal Kothari wrote:
> > Refactor nested if-else to avoid deep indentations. There is no change
> > in the logic of the new code, however, now it is simple because it gets
> > rid of five unnecessary else conditionals and it combines nested if into
> > single if-else-if. This refactor also leads to fix warning detected by
> > checkpatch.pl <http://checkpatch.pl>:
> > WARNING: Too many leading tabs - consider code refactoring
> >
> > Signed-off-by: Kushal Kothari <kushalkothari285@xxxxxxxxx <mailto:kushalkothari285@xxxxxxxxx>>
> > ---
> >
> > Changes in v2: Fix the bug of not handling properly the else logic
> > when p is not null in else-if. Also, reword the subject line and break
> > it up at 72 columns.
> >
> >  drivers/staging/rtl8723bs/core/rtw_mlme_ext.c | 69 ++++++++-----------
> >  1 file changed, 29 insertions(+), 40 deletions(-)
> >
> > diff --git a/drivers/staging/rtl8723bs/core/rtw_mlme_ext.c b/drivers/staging/rtl8723bs/core/rtw_mlme_ext.c
> > index 0f82f5031c43..267d853b1514 100644
> > --- a/drivers/staging/rtl8723bs/core/rtw_mlme_ext.c
> > +++ b/drivers/staging/rtl8723bs/core/rtw_mlme_ext.c
> > @@ -1192,50 +1192,39 @@ unsigned int OnAssocReq(struct adapter *padapter, union recv_frame *precv_frame)
> >               p = pframe + WLAN_HDR_A3_LEN + ie_offset; ie_len = 0;
> >               for (;;) {
> >                       p = rtw_get_ie(p, WLAN_EID_VENDOR_SPECIFIC, &ie_len, pkt_len - WLAN_HDR_A3_LEN - ie_offset);
> > -                     if (p) {
> > -                             if (!memcmp(p+2, WMM_IE, 6)) {
> > -
> > -                                     pstat->flags |= WLAN_STA_WME;
> > -
> > -                                     pstat->qos_option = 1;
> > -                                     pstat->qos_info = *(p+8);
> > -
> > -                                     pstat->max_sp_len = (pstat->qos_info>>5)&0x3;
> > -
> > -                                     if ((pstat->qos_info&0xf) != 0xf)
> > -                                             pstat->has_legacy_ac = true;
> > -                                     else
> > -                                             pstat->has_legacy_ac = false;
> > -
> > -                                     if (pstat->qos_info&0xf) {
> > -                                             if (pstat->qos_info&BIT(0))
> > -                                                     pstat->uapsd_vo = BIT(0)|BIT(1);
> > -                                             else
> > -                                                     pstat->uapsd_vo = 0;
> > -
> > -                                             if (pstat->qos_info&BIT(1))
> > -                                                     pstat->uapsd_vi = BIT(0)|BIT(1);
> > -                                             else
> > -                                                     pstat->uapsd_vi = 0;
> > -
> > -                                             if (pstat->qos_info&BIT(2))
> > -                                                     pstat->uapsd_bk = BIT(0)|BIT(1);
> > -                                             else
> > -                                                     pstat->uapsd_bk = 0;
> > -
> > -                                             if (pstat->qos_info&BIT(3))
> > -                                                     pstat->uapsd_be = BIT(0)|BIT(1);
> > -                                             else
> > -                                                     pstat->uapsd_be = 0;
> > -
> > -                                     }
> > -
> > -                                     break;
> > +                     if (p && memcmp(p+2, WMM_IE, 6)) {
> > +                             p = p + ie_len + 2;
> > +                     } else if (p && !memcmp(p+2, WMM_IE, 6)) {
> > +                             pstat->flags |= WLAN_STA_WME;
> > +                             pstat->qos_option = 1;
> > +                             pstat->qos_info = *(p+8);
> > +                             pstat->max_sp_len = (pstat->qos_info>>5)&0x3;
> > +
> > +                             pstat->has_legacy_ac = false;
> > +                             if ((pstat->qos_info&0xf) != 0xf)
> > +                                     pstat->has_legacy_ac = true;
> > +
> > +                             if (pstat->qos_info&0xf) {
> > +                                     pstat->uapsd_vo = 0;
>
> This variable and other variables below are set to 0 just before the loop,
> so the initialization here can be removed.
>
> > +                                     if (pstat->qos_info&BIT(0))
> > +                                             pstat->uapsd_vo = BIT(0)|BIT(1);
> > +
> > +                                     pstat->uapsd_vi = 0;
> > +                                     if (pstat->qos_info&BIT(1))
> > +                                             pstat->uapsd_vi = BIT(0)|BIT(1);
> > +
> > +                                     pstat->uapsd_bk = 0;
> > +                                     if (pstat->qos_info&BIT(2))
> > +                                             pstat->uapsd_bk = BIT(0)|BIT(1);
> > +
> > +                                     pstat->uapsd_be = 0;
> > +                                     if (pstat->qos_info&BIT(3))
> > +                                             pstat->uapsd_be = BIT(0)|BIT(1);
> >                               }
> > +                             break;
> >                       } else {
> >                               break;
> >                       }
> > -                     p = p + ie_len + 2;
> >               }
> >       }
>
> > --
> > 2.25.1
> >
>
> --
> Sincerely yours,
> Mike.
>